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
Add
EXCLUDE
to the parser, ast, plan, and plan schema inferencer #1226Add
EXCLUDE
to the parser, ast, plan, and plan schema inferencer #1226Changes from 10 commits
e158274
801437e
e7bc9ec
35ee02e
4370909
572e61d
75e516a
0adf26c
d60d08c
36065f0
7679e9b
ec6da34
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.
Do we allow
PIVOT x.v AT x.a EXCLUDE ...
?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 will be allowed. There's no tests yet since
PIVOT
is not yet fully implemented in the plan schema inferencer.I will add a parser test showing that it's allowed though.
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 if this would be required as part of the RFC, but If we wanted to make sure we don't come into a situation such as:
we could:
It should make sure that we don't accidentally break future queries. It's IMO better to error now even though we aren't sure what the RFC will say. Then, if we allow nonexistent bindings after the RFC, we can always remove the error? Then you can add the second test that @yliuuuu mentioned above.
LMK what you 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.
Also, it would surely help users who write:
Since we are specifically looking in the bindings coming from the SCAN, when we look for the
EXCLUDE a.b
(and specifically looking fora
), we are silently ignoring the fact thata
isn't in the EXCLUDE's input bindings. So, even though the projection list can usea
, theEXCLUDE
is silently letting this data to pass through. The current implementation, if I'm not mistaken, will return: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.
Since resolution of the
EXCLUDE
expressions is not defined, I think it's perfectly fine to mandate that users write... EXCLUDE t.a.b
explicitly. Though, if we silently let them writea.b
without notifying them thata
isn't a binding (and therefore they think that their query will stripb
), we'll be leaking data.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.
That's fair. I had originally wanted to punt on the erroring behavior to form a consistent mental model for when to give an error in the RFC. The more I think about it though, if the exclude expr root is not in the binding tuples, then we should give some error. I'll adjust to at least give an error in this case (i.e. when the root doesn't exist)
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.
Sanity check here.
So if we have
{'a' : [0, 1, 2]}
and excludea[0]
, the result will be{'a' : [1,2]}
, hence optional, i.e.,unionOf(missing, int)
.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.
Can you elaborate on this?
I would assume that the static type of
a
would be aListType(INT)
. IMO, even if we had:I would assume the static type of
t.a
would still beListType(INT)
because we can't know how long the list actually is (especially ift
is part of the catalog). Ift.a
started without any elements, I also think it would still beListType(INT)
. That being said @alancai98 , do we expect this to be a runtime error when we attempt to exclude an index that doesn't exist (essentiallyOutOfBoundsException
)? How about when we try to exclude an attribute in a tuple that doesn't exist? This one we can sometimes know statically, but what happens at runtime if it's a union type?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.
Not quite. In this case,
a
's type would beListType(INT)
. ThelastTypeAsOptional
is currently only used for when there's a subsequent field to exclude after the collection index (such asEXCLUDE t.a[0].field
-- in this case,field
's type would beunionOf(missing, <field's original type>)
).This is demonstrated by the below test case:
partiql-lang-kotlin/partiql-lang/src/test/kotlin/org/partiql/lang/planner/transforms/PartiQLSchemaInferencerTests.kt
Lines 1226 to 1285 in 4370909
^ In above,
c
's element type is not a union ofStructType(...)
andMISSING
.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.
Regarding runtime behavior, this hasn't yet been decided and will be part of the upcoming RFC. The schema inferencer doesn't currently give an error if an attribute doesn't exist. We can choose to add errors in the future. I can add this note to the
EXCLUDE
code.It currently gives a data type mismatch if the types doesn't match up:
^ will give an error
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 change was for fixing the
ORDER BY
schema inference behavior.Tests with corrected behavior:
partiql-lang-kotlin/partiql-lang/src/test/kotlin/org/partiql/lang/planner/transforms/PartiQLSchemaInferencerTests.kt
Lines 658 to 678 in e158274
Previously these tests would output a bag rather than a list.