Skip to content

Allow EXPLODE WITH ORDINALITY to flow 0-based ordinals - #4625

Merged
g31pranjal merged 2 commits into
mainfrom
apple/g31pranjal/unnested/opt-in-zero-based-ordinality
Sep 16, 2026
Merged

Allow EXPLODE WITH ORDINALITY to flow 0-based ordinals#4625
g31pranjal merged 2 commits into
mainfrom
apple/g31pranjal/unnested/opt-in-zero-based-ordinality

Conversation

@g31pranjal

@g31pranjal g31pranjal commented Sep 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

The ordinals EXPLODE ... WITH ORDINALITY flows are 1-based, as the SQL standard requires of WITH ORDINALITY, so expressing a position from it means subtracting one from an ordinal.

This PR allows the RecordQueryExplodePlan flow its ordinals 0-based when the caller asks for it. The choice is a property of the plan, serialized as zero_based_ordinality:

  • Unset or false means 1-based, which is what every plan serialized before this field existed flows, so an old plan keeps its meaning when a newer version reads it — a continuation created by an older server resumes on a newer one unchanged.
  • A plan that flows 1-based ordinals does not set the field at all, so it serializes to exactly the bytes, and hashes to exactly the plan hash, that it did before. SQL AT keeps asking for 1-based ordinals here, so no query plan that exists today changes.

Future PRs:

  1. Make SQL AT unnesting use 0-based explode ordinals #4617: Once the release is cut, we can switch the Relational to use 0-based ordinals when providing ordinals. This will be backward compatible in the sense that old plans continue to work on 1-based ordinals.
  2. TBD: After 1, all plans have true value for zero_based_ordinality. At this point, we change the semantics to treat unset zero_based_ordinality as 0-based. change plan to be unset.
  3. TBD: cleanup, planHash still needs to keep the remnants though.

@g31pranjal g31pranjal added the enhancement New feature or request label Sep 16, 2026
@g31pranjal
g31pranjal added this pull request to stack #4627 September 16, 2026 11:19
@g31pranjal
g31pranjal removed this pull request from stack #4627 September 16, 2026 13:16
@g31pranjal g31pranjal mentioned this pull request Sep 16, 2026
Merged
@g31pranjal
g31pranjal force-pushed the apple/g31pranjal/unnested/opt-in-zero-based-ordinality branch from 4d2d403 to 9b5aca6 Compare September 16, 2026 13:16
@g31pranjal
g31pranjal added this pull request to stack #4628 September 16, 2026 13:17
@g31pranjal g31pranjal changed the title Allow EXPLODE WITH ORDINALITY to flow 0-based ordinals Flow 0-based ordinals from EXPLODE WITH ORDINALITY, and use them for SQL AT Sep 16, 2026
@g31pranjal g31pranjal added breaking change Changes that are not backwards compatible Run mixed-mode Label to add to Pull Requests to have it run mixed mode tests and removed enhancement New feature or request labels Sep 16, 2026
@github-actions

github-actions Bot commented Sep 16, 2026

Copy link
Copy Markdown

📊 Metrics Diff Analysis Report

Summary

  • New queries: 0
  • Dropped queries: 0
  • Plan changed + metrics changed: 0
  • Plan unchanged + metrics changed: 0
ℹ️ About this analysis

This automated analysis compares query planner metrics between the base branch and this PR. It categorizes changes into:

  • New queries: Queries added in this PR
  • Dropped queries: Queries removed in this PR. These should be reviewed to ensure we are not losing coverage.
  • Plan changed + metrics changed: The query plan has changed along with planner metrics.
  • Metrics only changed: Same plan but different metrics

The last category in particular may indicate planner regressions that should be investigated.

@g31pranjal
Allow EXPLODE WITH ORDINALITY to flow 0-based ordinals
f1509aa 
An index defined on an unnested record type has to reconstruct the `__positions`
field of the synthetic records it covers, and positions are 0-based. The
ordinals `EXPLODE ... WITH ORDINALITY` flows are 1-based, as the SQL standard
requires of `WITH ORDINALITY`, so expressing a position means subtracting one
from an ordinal, and arithmetic in a candidate's values is arithmetic that has
to be matched away again on the query side.

So let the plan flow its ordinals 0-based when the caller asks for it. The
choice is a property of the plan, serialized as `zero_based_ordinality`:

  - Unset or false means 1-based, which is what every plan serialized before
    this field existed flows, so an old plan keeps its meaning when a newer
    version reads it -- a continuation created by an older server resumes on a
    newer one unchanged.
  - A plan that flows 1-based ordinals does not set the field at all, so it
    serializes to exactly the bytes, and hashes to exactly the plan hash, that
    it did before. SQL `AT` keeps asking for 1-based ordinals, so no query plan
    that exists today changes.

The ordinal base is part of a plan's and an expression's identity: the two
variants flow different ordinals for the same array, so neither may stand in
for the other, and the 0-based variant explains itself as
`WITH ZERO BASED ORDINALITY`.
@g31pranjal
g31pranjal removed this pull request from stack #4628 September 16, 2026 13:35
@g31pranjal
g31pranjal force-pushed the apple/g31pranjal/unnested/opt-in-zero-based-ordinality branch from 9b5aca6 to f1509aa Compare September 16, 2026 13:35
@g31pranjal g31pranjal added enhancement New feature or request and removed breaking change Changes that are not backwards compatible Run mixed-mode Label to add to Pull Requests to have it run mixed mode tests labels Sep 16, 2026
@g31pranjal g31pranjal changed the title Flow 0-based ordinals from EXPLODE WITH ORDINALITY, and use them for SQL AT Allow EXPLODE WITH ORDINALITY to flow 0-based ordinals Sep 16, 2026
@g31pranjal
g31pranjal added this pull request to stack #4629 September 16, 2026 13:36
@g31pranjal
g31pranjal marked this pull request as ready for review September 16, 2026 14:10
alecgrieser
alecgrieser approved these changes Sep 16, 2026
View reviewed changes

@alecgrieser alecgrieser left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I had a few minor comments that don't affect functional code. If you want this in to get it into a release sooner rather than later, they seem fine to do as a follow up

Comment thread ...in/java/com/apple/foundationdb/record/query/plan/cascades/expressions/ExplodeExpression.java
Comment thread ...ore/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java Outdated
Comment thread ...ore/src/main/java/com/apple/foundationdb/record/query/plan/plans/RecordQueryExplodePlan.java
Comment on lines -319 to +341
return withOrdinality ? PlanHashable.objectsPlanHash(mode, BASE_HASH, result, true)
: PlanHashable.objectsPlanHash(mode, BASE_HASH, result);
if (!withOrdinality) {
return PlanHashable.objectsPlanHash(mode, BASE_HASH, result);
}
return zeroBasedOrdinality ? PlanHashable.objectsPlanHash(mode, BASE_HASH, result, true, true)
: PlanHashable.objectsPlanHash(mode, BASE_HASH, result, true);

@alecgrieser alecgrieser Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes sense (for now). We may also want to guard a simpler creation of the hash (just: PlanHashable.objectsPlanHash(mode, BASE_HASH, result, withOrdinality, zeroBasedOrdinality)) on a new plan hash version so that we don't need to keep it like this forever.

@g31pranjal
g31pranjal merged commit 2da82fb into main Sep 16, 2026
14 checks passed
@g31pranjal
g31pranjal deleted the apple/g31pranjal/unnested/opt-in-zero-based-ordinality branch September 16, 2026 18:55
@g31pranjal g31pranjal mentioned this pull request Sep 17, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alecgrieser alecgrieser alecgrieser approved these changes

Assignees

No one assigned

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@g31pranjal @alecgrieser