Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement Table.replace for the database backend #8986
Implement Table.replace for the database backend #8986
Changes from 63 commits
756dfa9
58da2fd
f2cd94a
0501d05
fcab4e0
f2a58f6
5ed3fed
8caac00
74bc949
69753fa
4cf1309
aae2bd2
d458ed8
f924690
bf142c0
b97841c
b88633e
2a02eb0
2b7ad13
8b7c361
9a084d0
64d43f3
4acb007
df35418
5ac57ce
d01152d
69cdb47
76368e7
4ea9c22
a269166
b6cb8cc
940bfd3
bb7cb2f
6c1d530
a475905
f9aaf71
7ad9723
fe389f8
645805b
5450561
a6523c0
4dec328
d56bb09
947ebcf
8ad298d
e9c1a2c
324cbef
7a5854c
02a0d54
de5f91e
ea2876d
6fff368
80dbc76
8afbb74
e74b67d
4b34631
b241fbe
21b6b80
fd9212a
fa7c024
fbea5a9
0073e46
a5bf58a
5a1b6d0
ebf5087
cdddcc8
76ff386
22a76bb
0090a20
b99627c
0028687
5eaf502
5a772f0
2c47cfe
32452e8
9bce18d
bd3a300
4174e9b
ae66716
a1558b7
3aeb792
5c0a54a
6af697b
12a4c8e
88e022f
cd3fab6
046488c
2a7d8ae
fe72e3b
ea47095
7df7499
7cd0bb8
6a4ad4b
72f36c7
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
Array.transpose
have the same docs asVector.tranpose
? Or is it not exposed to the user?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should have on both as we make the APIs the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. How does an end-user actually make use of Arrays? I changed the docs for
Array
to sayArray
instead ofVector
, assuming that end-users are actually aware ofArray
as a distinct type, but is that right?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aren't these from the
lookup_table
so the selector is wrong?We need to have widgets derived from first argument for this I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the notation would be for this -- is there documentation for the @ clauses? Or, where is it implemented? I don't see an example of a widget attached to a value other than
self
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps this should be bigger?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any DB limit? I guess it is limited by the query size... I wonder if such literal table will be able to use hashjoins or will it always fall back to a linear scan. I guess for <256 it doesn't matter much. For larger values the size of the SQL query may start being quite problematic.
I think ideally we shouldn't cut off but instead we should be creating temporary tables - but definitely an improvement for a separate PR.
(I also wonder, given how often currently Enso re-evaluates the expressions, if we could create too much garbage in such way)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that was my concern. My first thought was to create a temporary table, but that will be happening repeatedly during each evaluation.
I was able to create tables with ~18000 rows this way in both postgres and sqlite, and at that point I stopped testing. I'm more concerned about the size of the query being sent over the wire, but I don't really know at what point that becomes a problem. I figured that giving it a small would be a good first step. I think it covers the majority of use-cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting that it worked for such large sizes.
I guess the primary concern indeed is the query size, especially as a temp table is likely sent in a more compact binary compact.
Anyway, seems all ok for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
256 a reasonable literal default. Agee with @radeusgd should be a temp table when gets bigger!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this result in a lot of temporary tables being created and not eagerly cleaned up? That was my concern when doing it the literal way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed that is a concern.
I think we should implement such a feature as a separate PR, as it has non-trivial complexity.
Also, I think the concern of 'too many' temporary tables may also be increased due to the fact that currently the IDE seems to recompute unrelated nodes 'too often'. I think that if we resolve that, the amount of re-computations could be lower and thus the problem would not be as big.
As for implementing these temporary tables efficiently, we can leverage a few tricks:
Managed_Resource
framework to try to clean up the tables once the references to them are GCed. We actually already implement a very similar feature -Hidden_Table_Registry
. It is used to be able to re-use temporary hidden tables for dry-run operations, and clean them up once they are no longer needed. We could extend this registry to also support such temporary tables that are not used for dry-run (and accessed by name) but accessed by e.g. content's hash.In fact we can merge the 2 approaches. If between runs the tables are the same we can avoid re-uploading them. Once all references to them are GCed we can clean them up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#9086