Repository navigation
Flow the WITH ORDINALITY struct as a record constructor value - #4696
Conversation
#4650 gave an explode the flag that decides whether it flows the element and the ordinal as a `RecordConstructorValue` or as one opaque value of the struct type, but left every explode flowing the opaque value. Ask for the record constructor where the SQL layer builds an `AT` unnesting, which is the only place that produces an explode with ordinality. The record-layer constructors keep defaulting to the opaque value, so nothing else changes shape. Both shapes stand for the same data and evaluate identically -- the plan builds its struct from the protobuf descriptor its declared type names -- but the element and the ordinal are now reachable sub-values, which is what lets anything be related to them. The expected plans of the five `AT` queries that also carry an `IN` move, since an `IN` over a value read out of a record is costed differently than one over an opaque value. They move in both directions: `array-join-at`'s `WHERE "at" IN (1, 2)` drops its join over the `IN` list for a single scan with a residual filter, two queries with an `IN` on the primary key keep their operators but move the unnesting across the in-join boundary, and `in-predicate`'s two queries with an `IN` on the unnested value start scanning the table once per `IN` element, which is #4621. No index match is lost, and all results still verify. A plan serialized before the flag existed still deserializes to the opaque value, so it keeps the shape, and the hash, it was planned with. A release that predates #4650 drops the flag instead, so this cannot be released before #4650 is.
g31pranjal
added
enhancement
alecgrieser
added
the
Run mixed-mode
alecgrieser
left a comment
alecgrieser
left a comment
There was a problem hiding this comment.
LGTM. I did point out some plan differences, including ones where the plan got worse. But there's another example of essentially the same transformation, but in reverse, which makes me think this is random noise from plan hash differences. Which, is maybe something to be aware of, but also hard to argue should be a blocking change, though potentially someone with more planner expertise should weigh in.
You did mark this as a
breaking change
| - query: SELECT t.id, n, idx FROM array_table AS t, t.numbers AS n AT idx WHERE n IN (10, 20) | ||
| - supported_version: 4.12.5.0 | ||
| - explain: "SCAN([IS ARRAY_TABLE]) | FLATMAP q0 -> { EXPLODE arrayDistinct(promote(@c23 AS ARRAY(LONG))) | FLATMAP q1 -> { EXPLODE q0.NUMBERS WITH ORDINALITY | FILTER _._0 EQUALS q1 AS q2 RETURN q2 } AS q2 RETURN (q0.ID AS ID, q2._0 AS N, q2._1 + 1 AS IDX) }" | ||
| - explain: "EXPLODE arrayDistinct(promote(@c23 AS ARRAY(LONG))) | FLATMAP q0 -> { SCAN([IS ARRAY_TABLE]) | FLATMAP q1 -> { EXPLODE q1.NUMBERS WITH ORDINALITY | FILTER _._0 EQUALS q0 AS q2 RETURN (q1.ID AS ID, q2._0 AS N, q2._1 + 1 AS IDX) } AS q3 RETURN q3 }" |
There was a problem hiding this comment.
This plan is worse, because we're now executing the SCAN([IS ARRAY_TABLE]) operator once per element in @c23 rather than once for the whole plan. I'm a little surprised that this change would cause that though unless this was relying on the plan hash tiebreaker or something.
| - query: SELECT t.id, f, idx FROM array_table AS t, t.fruits AS f AT idx WHERE f IN ('apple', 'mango') | ||
| - supported_version: 4.12.5.0 | ||
| - explain: "SCAN([IS ARRAY_TABLE]) | FLATMAP q0 -> { EXPLODE arrayDistinct(@c23) | FLATMAP q1 -> { EXPLODE q0.FRUITS WITH ORDINALITY | FILTER _._0 EQUALS q1 AS q2 RETURN q2 } AS q2 RETURN (q0.ID AS ID, q2._0 AS F, q2._1 + 1 AS IDX) }" | ||
| - explain: "EXPLODE arrayDistinct(@c23) | FLATMAP q0 -> { SCAN([IS ARRAY_TABLE]) | FLATMAP q1 -> { EXPLODE q1.FRUITS WITH ORDINALITY | FILTER _._0 EQUALS q0 AS q2 RETURN (q1.ID AS ID, q2._0 AS F, q2._1 + 1 AS IDX) } AS q3 RETURN q3 }" |
There was a problem hiding this comment.
This plan is also worse, for effectively the same reason.
| FROM T1, T1."arr1_nn" AS "val" AT "at" | ||
| WHERE "at" IN (1, 2) | ||
| - explain: "EXPLODE arrayDistinct(@c19) | FLATMAP q0 -> { SCAN([IS T1]) | FLATMAP q1 -> { EXPLODE q1.arr1_nn WITH ORDINALITY | FILTER _._1 + 1 EQUALS q0 AS q2 RETURN (q1.id AS id, q2._0 AS val, q2._1 + 1 AS at) } AS q3 RETURN q3 }" | ||
| - explain: "SCAN([IS T1]) | FLATMAP q0 -> { EXPLODE q0.arr1_nn WITH ORDINALITY | FILTER _._1 + 1 IN @c19 AS q1 RETURN (q0.id AS id, q1._0 AS val, q1._1 + 1 AS at) }" |
There was a problem hiding this comment.
For the sake of fairness, this plan is better, effectively because it makes the opposite transformation as: https://github.com/FoundationDB/fdb-record-layer/pull/4696/changes#r4166198242 Which is more evidence that this is random plan hash noise, I guess
📊 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: 2 queries Statistical Summary (Plan and Metrics Changed)
Significant Regressions (Plan and Metrics Changed)There were 2 outliers detected. Outlier queries have a significant regression in at least one field. Statistically, this represents either an increase of more than two standard deviations above the mean or a large absolute increase (e.g., 100).
|
This updates the project's minor version to 4.16. This is to accommodate the following breaking changes: * #4696 and #4704 - Breaks plan continuation compatibility for queryies using `WITH ORDINALITY` with versions prior to 4.15.2.0 * #4662 and #4663 - Removes two shaded artifacts from being published
#4650, now merged, added the
flowsRecordConstructorValueflag but left every explode flowing the opaque value. This PR turns it on only forEXPLODE WITH ORDINALS(LogicalOperator.generateCorrelatedFieldAccess). Wrapping the element of a plain explode in a struct of its own is #4623.What changes in plans
Nothing in
fdb-record-layer-corechanges, and no core test needed updating. What moves is the expected plan of the fiveATqueries that also carry anIN: anINover a value read out of a record is costed differently than one over an opaque value. The movement goes in both directions and is worth a look:array-join-at:317,WHERE "at" IN (1, 2): was an explode of theINlist joined with a scan, and is now a single scan with a residualFILTER _._1 + 1 IN @c19. Cheaper.array-join-at:305andin-predicate:402, bothINon the primary key: same operators, but the unnesting moves across the in-join boundary — into the in-join in the first, out of it in the second.in-predicate:382andin-predicate:410, bothINon the unnested value: theINlist explode becomes the outer loop with the table scan inside it, i.e. one full scan perINelement. This is Cost model scores a nested data access the same as an unnested one, so an IN-join can be chosen over a single scan #4621 — the cost model scores a data access nested under an in-join the same as an unnested one.