Remove multiplicity from QueryUnit.sql
#7985
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Currently, `QueryUnit.sql` is a tuple representing, possibly, multiple SQL statements corresponding to the original EdgeQL statement. This introduces significant complexity to consumers of `QueryUnit`, which are mostly unprepared to handle more than one SQL statement anyway. Originally the SQL tuple was used to represent multiple EdgeQL statements, a task which is now handled by the `QueryUnitGroup` stuff. Another use case are the non-transactional commands (`CREATE BRANCH` and friends). Finally, since this facility was available, more uses of it were added without actually _needing_ be executed as multiple SQL statements with no other recourse: those are mostly maintenance commands and the DDL type_id readback. I think the above uses are no longer a sufficient reason to keep the tuple complexity and so I'm ripping it out here, making `QueryUnit.sql` a `bytes` property with the invariant of _always_ containing exactly one SQL statement and thus not needing any special handling. The users are fixed up as follows: - Non-transactional branch units encode the extra SQL needed to represent them in the new `QueryUnit.db_op_trailer` property which is a tuple of SQL. Branch commands have special handling for them already, so this is not a nuisance. - The newly-added type id mappings produced by DDL commands are now communicated via the new "indirect return" mechanism, whereby a DDL PLBlock can communicate a return value via a specially-formatted `NoticeResponse` message. - All other unwitting users of multi-statement `QueryUnit` are converted to use a single statement instead (primarily by converting them to use a `DO` block).
elprans
force-pushed
the
query-unit-cleanup
branch
from
ebfb72a to
c6a7040
Compare
msullivan
approved these changes
Nov 12, 2024
Comment on lines
+740
to
+745
| "statements": [{"text": stmt} for stmt in self.statements], | ||
| "confidence": self.confidence, | ||
| "prompt": self.prompt, | ||
| "prompt_id": self.prompt_id, | ||
| "data_safe": self.data_safe, | ||
| "required_user_input": list(self.required_user_input), |
There was a problem hiding this comment.
It's probably not worth undoing but all these automated changes make this diff pointlessly noisy
There was a problem hiding this comment.
Not sure how this happened, I don't have format-on-save enabled.
There was a problem hiding this comment.
You clearly have something enabled, plenty of changes here just change ' to "
There was a problem hiding this comment.
Must have pressed a "format file" shortcut or something. It's just this file.
1st1
approved these changes
Nov 12, 2024
Yeah, it does seem like it at first, but the mechanism is reliable. There are more cleanups to be realized from the single-statement invariant of |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Currently,
QueryUnit.sqlis a tuple representing, possibly, multipleSQL statements corresponding to the original EdgeQL statement. This
introduces significant complexity to consumers of
QueryUnit, which aremostly unprepared to handle more than one SQL statement anyway.
Originally the SQL tuple was used to represent multiple EdgeQL statements,
a task which is now handled by the
QueryUnitGroupstuff. Another usecase are the non-transactional commands (
CREATE BRANCHand friends).Finally, since this facility was available, more uses of it were added
without actually needing be executed as multiple SQL statements
with no other recourse: those are mostly maintenance commands and the
DDL type_id readback.
I think the above uses are no longer a sufficient reason to keep the
tuple complexity and so I'm ripping it out here, making
QueryUnit.sqla
bytesproperty with the invariant of always containing exactly oneSQL statement and thus not needing any special handling. The users are
fixed up as follows:
Non-transactional branch units encode the extra SQL needed to
represent them in the new
QueryUnit.db_op_trailerproperty which isa tuple of SQL. Branch commands have special handling for them
already, so this is not a nuisance.
The newly-added type id mappings produced by DDL commands are now
communicated via the new "indirect return" mechanism, whereby a DDL
PLBlock can communicate a return value via a specially-formatted
NoticeResponsemessage.All other unwitting users of multi-statement
QueryUnitare convertedto use a single statement instead (primarily by converting them to use a
DOblock).