Repository navigation
Stop supporting 1-based explode ordinals - #4704
Merged
g31pranjal merged 3 commits intoOct 2, 2026
Merged
Conversation
g31pranjal
added
enhancement
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/default-zero-based-ordinality
branch
6 times, most recently
from
October 2, 2026 12:02
a69a46d to
5af9ce4
Compare
g31pranjal
changed the title
g31pranjal
marked this pull request as ready for review
October 2, 2026 12:05
alecgrieser
added
the
Run mixed-mode
alecgrieser
requested changes
Oct 2, 2026
alecgrieser
left a comment
alecgrieser
left a comment
Collaborator
There was a problem hiding this comment.
The substantive changes look good to me. This will break compatibility with 4.14's continuations (as we see in the mixed mode tests), so we should update the minor version. In theory, that could be done in a separate PR, but we should remember to do that before we do a release. A few other things, though the main non-test comment is maybe more food for thought
robert-brunel
requested changes
Oct 2, 2026
`EXPLODE ... WITH ORDINALITY` flowed 1-based ordinals until #4625 added a flag to ask for 0-based ones, and #4633 turned that flag on for every plan the SQL layer builds. Nothing asks for 1-based ordinals any more, so drop them: an explode always flows 0-based ordinals, and the ordinal base is gone from the planner API. A plan that asks for 1-based ordinals outright is rejected rather than resumed with ordinals it did not ask for. A plan that says nothing about the ordinal base is read as 0-based, which is the only base there is. `zero_based_ordinality` no longer carries a choice, but a `WITH ORDINALITY` plan keeps setting it: a version that predates this change reads the ordinal base off it, and would execute the plan 1-based without it. Once no supported version reads the field, it can stop being set, and then be removed. Continuations from before 4.15.1.0 cannot be resumed for `AT` queries, so the `AT` tests in array-join-at.yamsql and in-predicate.yamsql now require that version.
g31pranjal
force-pushed
the
apple/g31pranjal/unnested/default-zero-based-ordinality
branch
from
October 2, 2026 14:56
5af9ce4 to
c768731
Compare
Adjust the tests to the stricter rule: an unset zero_based_ordinality is now rejected rather than read as 0-based, and the fixture standing in for a plan from before flows_record_constructor_value existed has to mark its ordinals 0-based to get past the guard. Restore the original supported_version floors on the AT queries, so that mixed mode reports their incompatibility with older versions instead of skipping past it.
alecgrieser
approved these changes
Oct 2, 2026
robert-brunel
approved these changes
Oct 2, 2026
g31pranjal
deleted the
apple/g31pranjal/unnested/default-zero-based-ordinality
branch
October 2, 2026 18:07
alecgrieser
added a commit
that referenced
this pull request
Oct 5, 2026
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
g31pranjal
added a commit
that referenced
this pull request
Oct 8, 2026
This was referenced Oct 21, 2026
g31pranjal
added a commit
that referenced
this pull request
Oct 21, 2026
Explode ordinals are always 0-based since #4704, and every version from 4.16 on sets zero_based_ordinality to true on WITH ORDINALITY plans. Deserializing an explode plan no longer reads the field, so the next release can stop setting it and drop it from PRecordQueryExplodePlan.
g31pranjal
added a commit
that referenced
this pull request
Oct 21, 2026
Explode ordinals are always 0-based since #4704. That PR kept the `zero_based_ordinality` field on `PRecordQueryExplodePlan` for compatibility, and planned to retire it over two releases: 4.17 stops reading it, and 4.18 stops setting it and drops it from the protobuf. This PR is the first step. `RecordQueryExplodePlan.fromProto` no longer reads `zero_based_ordinality`. Every version 4.16 onwards only writes `WITH ORDINALITY` plans with the field set to `true`, so there is nothing left for the check to reject. `toProto` keeps setting the field, because 4.16 still reads it and rejects a `WITH ORDINALITY` plan without it. This PR also bumps the minor version to 4.17. With the check gone, a plan written by 4.14 or earlier, which leaves the field unset and flows 1-based ordinals, would be read as 0-based instead of being rejected. So this change must not ship in a 4.16 release. Stopping to set the field, and dropping it from the protobuf, is tracked in #4769.
g31pranjal
added a commit
that referenced
this pull request
Oct 21, 2026
A WITH ORDINALITY explode plan hashes a second `true`, for the 0-based ordinals that were still a choice before #4704. VL0 and VC0 keep hashing it, so existing continuations still resume; VC1 no longer does.
alecgrieser
pushed a commit
that referenced
this pull request
Oct 22, 2026
…ans (#4768) A `WITH ORDINALITY` explode plan hashes `(result, true, true)`. The second `true` stood for the 0-based ordinals that were still a choice before #4704. Since #4704 ordinals are always 0-based, and that `true` was kept only so that plan hashes, and therefore continuations, would not change. This PR drops it from the `VC1` plan hash, the next continuation plan hash version. `VL0` and `VC0` keep hashing it, so existing continuations still resume. `CURRENT_FOR_CONTINUATION` is still `VC0`, so no hash in use today changes. - `RecordQueryExplodePlan.planHash` switches on the mode: `VL0` and `VC0` hash `(result, true, true)`, and `VC1` hashes `(result, true)`. A plain explode hashes the same in every mode. - `PlanHashable.PlanHashMode.VC1` lists the change in its javadoc. - `ExplodePlanTest.planHashIsStable` pins the `VC1` hashes. It also asserts that `VC1` differs from `VC0` for a `WITH ORDINALITY` explode, and that the plain and `WITH ORDINALITY` variants still hash differently under `VC1`.
This file contains hidden or 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
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.
EXPLODE ... WITH ORDINALITYflowed 1-based ordinals until #4625 added a flag to ask for 0-based ones, and #4633 turned that flag on for every plan the SQL layer builds. Nothing asks for 1-based ordinals any more as of release version4.15.3.0. This PR, hence, drops them: an explode always flows 0-based ordinals.What a deserialized plan may ask for
with_ordinalityzero_based_ordinalityA
WITH ORDINALITYplan has to say explicitly that its ordinals are 0-based. The other two shapes both flowed 1-based ordinals — one by asking for them outright, the other by predating the field — and neither is reinterpreted as 0-based; both fail withexplode plans flowing 1-based ordinals are no longer supported.Compatibility
zero_based_ordinalityno longer carries a choice, but aWITH ORDINALITYplan keeps setting it, because a version that predates this change reads the ordinal base off it. Continuations taken from before 4.15.1.0 can no longer be resumed forATqueries. TheATtests keep their existingsupported_versionfloors, so that mixed-mode runs report that incompatibility per version rather than skipping past it.Requiring the field to be set explicitly means it takes two releases to retire rather than one: 4.17 can stop reading it, and 4.18 can stop setting it and drop it from the protobuf.