-
Notifications
You must be signed in to change notification settings - Fork 18
Support selecting wildcards in node queries #993
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for thriving-cassata-78ae72 canceled.
|
5cf0317 to
3a4253e
Compare
agorajek
left a comment
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.
Nice one! Thank you.
| # Check if any columns have been updated | ||
| existing_columns = {col.name: col for col in node.current.columns} # type: ignore | ||
| updated_columns = False | ||
| updated_columns = len(current_node_revision.columns) != len(node_validator.columns) |
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.
nice catch!
| ), | ||
| ) | ||
| wildcard_parent.projection.remove(self) | ||
| else: |
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.
nit: it may be easier to invert this conditional:
if not isinstance(self.parent, Select):
return super().compile(ctx)
# and then the main code w/o the need to be indented
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.
Good point!
299d00d to
ce182ec
Compare
| for node_name in node_names: | ||
| impacts[node_name] = await hard_delete_node( | ||
| node_name, | ||
| for node in reversed(topological_sort(nodes)): |
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.
While we don't have to do a topological sort here, doing so will mean that we process the deletes for nodes in the namespace in an order that minimizes unnecessary invalidation followed by deletion.
| display_name=labelize(column_name), | ||
| type=column_type, | ||
| attributes=existing_column.attributes if existing_column else [], | ||
| attributes=[ |
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.
This was an annoying issue: the attributes field on the Column database object is actually a ColumnAttribute object and not an Attribute object. Since ColumnAttribute is the relational link between an Attribute and a Column, we actually need to recreate this for any copied columns. Otherwise, it'll cause problems downstream since it may reference a column that doesn't exist.
ba853a0 to
d51de7c
Compare
d51de7c to
0734396
Compare
|
@shangyian what happened here? Was there a major issue or just a missed PR? I see that we can create nodes with |
|
@agorajek yeah, unfortunately I couldn't get one of the unit tests to pass (there was some non-deterministic issue that was hard to track down). I'll take another stab at it soon. |
Summary
This PR adds support for writing
SELECT *in node queries.During the
Wildcardcompile stage, it expands outSELECT *to all available columns on the table references. This may include DJ node columns, CTE columns, or subquery columns. These expanded columns are saved to a node revision's associatedcolumns, but in the saved query, we keep the wildcard syntax (i.e., we always preserve what the user wrote).During node updates, we also update these columns based on the parent nodes' columns, if any has changed.
Test Plan
make checkpassesmake testshows 100% unit test coverageDeployment Plan