Repository navigation
Match a plain explode against an explode with ordinality - #4619
g31pranjal wants to merge 2 commits into
Conversation
e93765d to
5972e75
Compare
457737a to
802b25c
Compare
9186c7b to
d4ebb50
Compare
802b25c to
66106af
Compare
d4ebb50 to
70913fd
Compare
66106af to
cd8ca19
Compare
70913fd to
51f14cb
Compare
cd8ca19 to
fe9b6d2
Compare
51f14cb to
40cfe34
Compare
fe9b6d2 to
4e19391
Compare
40cfe34 to
28a0ea8
Compare
4e19391 to
4fb7006
Compare
c597cea to
0848c7b
Compare
4fb7006 to
b7db6bc
Compare
0848c7b to
69399ce
Compare
b7db6bc to
1818a02
Compare
69399ce to
b1a40b3
Compare
📊 Metrics Diff Analysis ReportSummary
ℹ️ About this analysisThis automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:
The last category in particular may indicate planner regressions that should be investigated. Plan and Metrics ChangedThese queries experienced both plan and metrics changes. This generally indicates that there was some planner change Total: 4 queries Statistical Summary (Plan and Metrics Changed)
There were no queries with significant regressions detected. Minor Changes (Plan and Metrics Changed)In addition, there were 4 queries with minor changes. |
730f88a to
7493cd5
Compare
efc2577 to
e11caaf
Compare
7493cd5 to
88b0d18
Compare
e11caaf to
44b5766
Compare
88b0d18 to
f3ab945
Compare
44b5766 to
597d82e
Compare
f3ab945 to
d28361e
Compare
597d82e to
647841e
Compare
An `EXPLODE WITH ORDINALS`'s value is a single opaque `QueriedValue`, even though it flows 2 pieces of information - element and its position. Nothing inside it is reachable, which is fine for evaluation but it leaves no way to relate the element to anything. ### Why `RecordConstructorValue` is needed: An index utilizing an `UnnestedRecordType` requires that the `SyntheticRecordType` is expanded into a candidate. This expansion in turn requires unnesting with ordinality, because the proto descriptor of the `UnnestedRecordType`, as well as its primary key, carries the position of the element. A query that unnests the same collection but asks for no ordinals must be matched against this candidate, by relating the query's element to the candidate's element: `MaxMatchMap` has to find the query's value among the candidate's reachable sub-values. `MaxMatchMap` descends only into record constructors. An opaque `QueriedValue` of the struct type cannot be descended into. Having `RecordConstructorValue`, makes the element a reachable sub-value and the match findable structurally. This matching happens in #4619; the shape it needs is turned on in #4651. ### Why the shape is recorded rather than inferred The value a plan flows is part of the plan's identity: `RecordQueryExplodePlan` hashes it, so changing the shape changes the plan hash of every `WITH ORDINALITY` plan. A plan serialized by an earlier version flows the opaque value and was hashed as such, and it has to keep doing both when a newer version reads it back. Deciding the shape from `withOrdinality` alone would hence not be backward-compatible. So the shape becomes state the plan records and serializes. This PR adds that state and leaves it at the value every existing plan flows; #4651 changes what newly planned explodes ask for. ### What this PR does `ExplodeExpression` and `RecordQueryExplodePlan` gain a `flowsRecordConstructorValue` flag. What the flag decides differs between the two variants. With ordinality the struct is the result type either way, so only the shape of the value changes: an older version that cannot see the flag still executes such a plan identically. Plainly, the struct is the flag's doing — the result type is the element type without it and a one-column struct with it — so the plan produces that struct rather than the bare element. That is why each variant needs its own PR to turn the shape on: #4651 for the ordinality one, and #4623 for the plain one, which reaches into everything that reads what an explode flows. **Nothing asks for new shape yet.** That will be enabled in later PRs ### Compatibility The flag is serialized in porto. Absent means the opaque value, which is precisely what a plan serialized before the field existed flows, so such a plan keeps both its shape and its hash.
c0e7a3a to
f456ca6
Compare
647841e to
a130c4c
Compare
An index defined on an `UnnestedRecordType` had no match candidate, so the Cascades planner could never use it. Build one from the graph #4626 expands the type into, and teach the relational layer to declare such a type from SQL: a synthetic table whose constituents come from unnesting a repeated field, the metadata to serialize it, and the DDL to define an index over it. The query side is a plain explode of the same repeated field, and it matches the candidate's explode with ordinality through #4619, whose mapping reaches the element because the candidate flows it as a column of a record constructor. `unnested-record-type-indexes.yamsql` covers the whole path end to end, from declaring the type in a schema template to a covering scan of the index. Squashed from the pre-split branch and re-rooted onto #4619, so that #4626 -> #4651 -> #4619 -> #4641 is linear.
f456ca6 to
609becf
Compare
a130c4c to
e35f8bf
Compare
An index defined on an `UnnestedRecordType` had no match candidate, so the Cascades planner could never use it. Build one from the graph #4626 expands the type into, and teach the relational layer to declare such a type from SQL: a synthetic table whose constituents come from unnesting a repeated field, the metadata to serialize it, and the DDL to define an index over it. The query side is a plain explode of the same repeated field, and it matches the candidate's explode with ordinality through #4619, whose mapping reaches the element because the candidate flows it as a column of a record constructor. `unnested-record-type-indexes.yamsql` covers the whole path end to end, from declaring the type in a schema template to a covering scan of the index. Squashed from the pre-split branch and re-rooted onto #4619, so that #4626 -> #4651 -> #4619 -> #4641 is linear.
609becf to
65b6627
Compare
e35f8bf to
5e8fdae
Compare
| */ | ||
| @Test | ||
| void plainExplodeIsSubsumedByAnOrdinalityExplodeFlowingARecordConstructor() { | ||
| // A string element, so that the element is told apart from the ordinal by its type alone. |
There was a problem hiding this comment.
So what would happen if the element type has exactly the same type as the ordinal (namely INT NOT NULL)? Would be good to have a test for that case too.
|
|
||
| if (!withOrdinality | ||
| && candidateExpression instanceof final ExplodeExpression candidateExplodeExpression | ||
| && candidateExplodeExpression.isWithOrdinality()) { |
There was a problem hiding this comment.
Also && candidateExplodeExpression.flowsRecordConstructorValue()?
| /** | ||
| * Two queried values are equal if they stand for streams of the same type. | ||
| */ | ||
| @Nonnull | ||
| @Override | ||
| public ConstrainedBoolean equalsWithoutChildren(@Nonnull final Value other) { | ||
| return LeafValue.super.equalsWithoutChildren(other) | ||
| .filter(ignored -> resultType.equals(((QueriedValue)other).resultType)); | ||
| } |
There was a problem hiding this comment.
This change looks suspicious. If I understand correctly, the root issue we have is that ExplodeExpression produces a RecordConstructorValue(QueriedValue(element), QueriedValue(INT)), and those two QueriedValue() cannot properly be distinguished inside pullUp/MaxMatchMap. So by adding this comparison by type here, we’re now mostly able to distinguish them? However, what if the type of element is INT, then this will break again.
I haven’t fully grokked this yet, but it sounds like we might have to introduce a dedicated leaf class for those ordinals. Then the existing comparison that looks at getClass() will be able to tell apart the ordinal and the element properly.
There was a problem hiding this comment.
I think what would happen without this added method is that semanticEquals() views these as equal when in fact they are not. I think this is a general bug and should be fixed. I think this is needed in this PR because we use QueriedValue for the ordinality.
So I thin the fix for this problem is ok.
I don't know where else to put the following: I think @robert-brunel is right that this is insufficient for matching since queried value really does not tell you that it is iterating over a sequence an produce the numbers. In general the modeling used for this case is wrong I think. If you say the result value is (QV(elementType), QV(INT)) that's incorrect. This indicates that this will create a record of a list of elements and a list of numbers when in fact it is creating a list of pairs of these things.
I think this is already done in a shady way when we are just dealing with the exploded value in the simple case. It should not be a RCV(something we iterate over).
The way out of this is to use the StreamingValue which is used for table functions. Regardless what's going on inside this one will always give you a value you stream over. You can then ask for the constituents of the items you stream over in order to do equalities and such.
So we can model it like this StreamingValue(q1.a, RCV(_.a, ordinalValue(_))) or maybe just like RCV(_.a, ordinalValue(_)). In both cases, the _ is the evaluation of the current element inside of the explode. This can be done using the current quantifier alias. Which we could reappropriate to do this.
In the end you can model simple ExplodeExpression like this StreamingValue(q1.a, _) and something more complex with ordinals and subsequent field access _per element like this: StreamingValue(q1.reviews, RCV(_.rating, ordinal(_))
| } | ||
|
|
||
| final var maxMatchMap = | ||
| MaxMatchMap.compute(getResultValue(), candidateExpression.getResultValue(), |
There was a problem hiding this comment.
They have to be identical, don't they? If so, this is pretty heavy-handed. I am not sure that there is a better API to create this M3 here. It would be nice to have something like MaxMatchMap.shortcircuitMaybe() for this case as well. Can you think of a scenario where we are matching something that is not dead-identical? In any case this should work, albeit it's inefficient.
| @Override | ||
| public ConstrainedBoolean equalsWithoutChildren(@Nonnull final Value other) { | ||
| return LeafValue.super.equalsWithoutChildren(other) | ||
| .filter(ignored -> resultType.equals(((QueriedValue)other).resultType)); |
There was a problem hiding this comment.
Can we pull this into Value itself?
There was a problem hiding this comment.
This is now not needed
| /** | ||
| * Two queried values are equal if they stand for streams of the same type. | ||
| */ | ||
| @Nonnull | ||
| @Override | ||
| public ConstrainedBoolean equalsWithoutChildren(@Nonnull final Value other) { | ||
| return LeafValue.super.equalsWithoutChildren(other) | ||
| .filter(ignored -> resultType.equals(((QueriedValue)other).resultType)); | ||
| } |
There was a problem hiding this comment.
I think what would happen without this added method is that semanticEquals() views these as equal when in fact they are not. I think this is a general bug and should be fixed. I think this is needed in this PR because we use QueriedValue for the ordinality.
So I thin the fix for this problem is ok.
I don't know where else to put the following: I think @robert-brunel is right that this is insufficient for matching since queried value really does not tell you that it is iterating over a sequence an produce the numbers. In general the modeling used for this case is wrong I think. If you say the result value is (QV(elementType), QV(INT)) that's incorrect. This indicates that this will create a record of a list of elements and a list of numbers when in fact it is creating a list of pairs of these things.
I think this is already done in a shady way when we are just dealing with the exploded value in the simple case. It should not be a RCV(something we iterate over).
The way out of this is to use the StreamingValue which is used for table functions. Regardless what's going on inside this one will always give you a value you stream over. You can then ask for the constituents of the items you stream over in order to do equalities and such.
So we can model it like this StreamingValue(q1.a, RCV(_.a, ordinalValue(_))) or maybe just like RCV(_.a, ordinalValue(_)). In both cases, the _ is the evaluation of the current element inside of the explode. This can be done using the current quantifier alias. Which we could reappropriate to do this.
In the end you can model simple ExplodeExpression like this StreamingValue(q1.a, _) and something more complex with ordinals and subsequent field access _per element like this: StreamingValue(q1.reviews, RCV(_.rating, ordinal(_))
A query that explodes a collection without preserving its order had no correspondence to a candidate that explodes the same collection preserving its order, such as the one an index on an unnested record type is expanded with, and any such match was lost: `TableFunctionExpression.exactlySubsumedBy` compares the explodes, which differ in `preserveOrder`. `subsumedBy` gains the case itself: both explodes flow the same `(element, ordinal)` struct, and the query does not observe its ordinals, so the candidate produces everything the query may produce. The match is otherwise built as `exactlySubsumedBy` builds it. A sorted query explode is excluded, as the candidate does not produce its elements in that order.
…nTest Covers the subsumption of an explode that does not preserve its order by one that does over the same collection, and the cases that must not match: the reverse direction, a sorted query explode, another collection, and a collection correlated to an unbound alias. A match maps the query's whole (element, ordinal) struct to the candidate's.
Builds on #4696, which makes an explode
WITH ORDINALITYflow a spelled-out(element, ordinal)record.A query that explodes a collection plainly had no correspondence to a candidate that explodes the same collection with ordinality, and any such match was lost:
exactlySubsumedBycomparesisWithOrdinality(), which the two never agree on.subsumedBygains the case itself: a plain explode is subsumed by an explode with ordinality over the same collection, since the candidate produces at least everything the query may produce. The mapping comes fromMaxMatchMap, which descends into the candidate's record constructor and finds this expression's element value as its first column, so the correspondence pulls up through the enclosing quantifiers asq._0rather than being lost. A candidate that flows the opaque value instead offers nothing to reach, and the match it yields carries an empty mapping — which is what the new unit test pins down.Because a
QueriedValuewas equal to every otherQueriedValueregardless of type, the two columns of an(element, ordinal)record were indistinguishable whenever the element is anINT, and pulling a value up through the explode found both.QueriedValue.equalsWithoutChildrennow also compares the result type.This is what lets an index defined over an
UnnestedRecordTypebe matched by a query that unnests the same collection without asking for ordinals.