Skip to content

Fix Cascades planner treating IS NOT DISTINCT FROM as unusable for index scans - #4598

Merged
pengpeng-lu merged 7 commits into
FoundationDB:mainfrom
pengpeng-lu:debug_distinct
Sep 11, 2026
Merged

Fix Cascades planner treating IS NOT DISTINCT FROM as unusable for index scans#4598
pengpeng-lu merged 7 commits into
FoundationDB:mainfrom
pengpeng-lu:debug_distinct

Conversation

@pengpeng-lu

Copy link
Copy Markdown
Contributor

Summary

  • ScanComparisons.getComparisonType and RangeConstraints classified NOT_DISTINCT_FROM as ComparisonType.NONE instead of EQUALITY, so it could never be folded into an index scan's bounds — it always fell back to a residual FILTER on top of a full/covering scan.
  • when the NOT_DISTINCT_FROM comparison sits on the leading key column of
    an index whose ordering doesn't otherwise match the query's ORDER BY, the planner has no way to resolve that
    leading column to a bound value — and the "just scan everything" fallback's natural order doesn't satisfy the
    requested order either. With no viable candidate plan, the Cascades planner throws UnableToPlanException
    ("Cascades planner could not plan query").
  • Fix: treat NOT_DISTINCT_FROM as ComparisonType.EQUALITY in ScanComparisons, and add it to RangeConstraints's
    equality-range handling (Range.singleton(...))
  • ComparisonTypes. IS_DISTINCT_FROM (its negation) is explicitly left classified as NONE — like NOT_EQUALS, it can't be expressed as a contiguous range, so it correctly remains a residual filter.

Test

  • scoped-keyset-pagination.yamsql: end-to-end regression test reproducing the actual UnableToPlanException scenario — a keyset-pagination OR-range on trailing index columns combined with a
    genuinely unbound IS NOT DISTINCT FROM parameter on the index's leading key column. fails with UnableToPlanException pre-fix, passes with the correct union-of-covering-scans plan post-fix.

@pengpeng-lu pengpeng-lu added the bug fix Change that fixes a bug label Sep 8, 2026
Comment on lines +1 to +28
# Regression test for a Cascades planner bug: a keyset-pagination OR-range on trailing index
# columns, combined with a genuinely unbound "IS NOT DISTINCT FROM" parameter on the index's
# LEADING key column, could fail to plan at all (UnableToPlanException) rather than producing a
# (possibly suboptimal) plan.
#
# The scoped_item table models a common multi-tenant shape: rows are partitioned by an optional
# "scope_id" (e.g. a tenant/zone), and callers query within one scope (or "scope-wide", across
# all scopes) using a null-safe equality check ("scope_id IS NOT DISTINCT FROM row's scope"),
# because the scope value itself may be null (a "default"/unscoped row).
#
# The scoped_item_scope_ts_idx index is rooted at (scope.scope_id, ts, id) - scope_id is the
# LEADING key column, not the trailing one. A full/unbounded scan of this index is therefore
# ordered by (scope_id, ts, id), NOT by (ts, id) alone - so it can only satisfy
# "ORDER BY ts DESC, id DESC" (which never mentions scope_id) if scope_id first resolves to a
# single equality value. That resolution requires recognizing "IS NOT DISTINCT FROM" as an
# equality-shaped range boundary. Without that, neither a bounded-range plan nor the "just scan
# everything" fallback is available (the fallback's natural order doesn't match ORDER BY either) -
# hence no plan at all.
#
# Critically, the "IS NOT DISTINCT FROM" comparison must be a genuinely unbound parameter, not a
# literal: a NULL-valued bind parameter gets rewritten to a plain "IS NULL" comparison before the
# planner ever sees a "NOT_DISTINCT_FROM" node at all (that's always been a valid equality-shaped
# comparison), so it never exercises the bug. A non-null bound value stays a genuine
# NOT_DISTINCT_FROM comparison against a real parameter reference.
#
# supported_version is pinned to the current (in-development) version: this fix hasn't shipped in
# any released version yet, so running this test against an older external server build would
# correctly (from that older build's own perspective) fail - that's not a regression to catch.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you shorten this AI please?

@normen662 normen662 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The PRB seems to be failing

@github-actions

Copy link
Copy Markdown

📊 Metrics Diff Analysis Report

Summary

  • New queries: 1
  • Dropped queries: 0
  • Plan changed + metrics changed: 5
  • 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.

New Queries

Count of new queries by file:

  • yaml-tests/src/test/resources/scoped-keyset-pagination.metrics.yaml: 1

Plan and Metrics Changed

These queries experienced both plan and metrics changes. This generally indicates that there was some planner change
that means the planning for this query may be substantially different. Some amount of query plan metrics change is expected,
but the reviewer should still validate that these changes are not excessive.

Total: 5 queries

Statistical Summary (Plan and Metrics Changed)

task_count:

  • Average change: -8.2
  • Median change: -3
  • Standard deviation: 10.4
  • Range: -29 to -3
  • Queries changed: 5
  • No regressions! 🎉

task_count % change distribution (5 queries, bin = 2%):

     Range                                  n
----------  ------------------------------  ---
[-4%, -2%)  ████████                        1
[-2%, +0%)  ██████████████████████████████  4

transform_count:

  • Average change: -3.4
  • Median change: -1
  • Standard deviation: 4.8
  • Range: -13 to -1
  • Queries changed: 5
  • No regressions! 🎉

transform_count % change distribution (5 queries, bin = 2%):

     Range                                  n
----------  ------------------------------  ---
[-8%, -6%)  ████████                        1
[-6%, -4%)                                  0
[-4%, -2%)                                  0
[-2%, +0%)  ██████████████████████████████  4

transform_yield_count:

  • Average change: +0.2
  • Average regression: +1.0
  • Median change: +1
  • Median regression: +1
  • Standard deviation: 1.6
  • Standard deviation of regressions: 0.0
  • Range: -3 to +1
  • Range of regressions: +1 to +1
  • Queries changed: 5
  • Queries regressed: 4

transform_yield_count % change distribution (5 queries, bin = 2%):

     Range                                  n
----------  ------------------------------  ---
[-6%, -4%)  ████████                        1
[-4%, -2%)                                  0
[-2%, +0%)                                  0
[+0%, +2%)                                  0
[+2%, +4%)  ██████████████████████████████  4

insert_new_count:

  • Average change: -3.8
  • Median change: -4
  • Standard deviation: 0.4
  • Range: -4 to -3
  • Queries changed: 5
  • No regressions! 🎉

insert_new_count % change distribution (5 queries, bin = 2%):

     Range                                  n
----------  ------------------------------  ---
[-8%, -6%)  ██████████████████████████████  4
[-6%, -4%)                                  0
[-4%, -2%)  ████████                        1

insert_reused_count:

  • Average change: -1.0
  • Median change: -1
  • Standard deviation: 0.0
  • Range: -1 to -1
  • Queries changed: 1
  • No regressions! 🎉

There were no queries with significant regressions detected.

Minor Changes (Plan and Metrics Changed)

In addition, there were 5 queries with minor changes.

@pengpeng-lu
pengpeng-lu merged commit 7cefc75 into FoundationDB:main Sep 11, 2026
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug fix Change that fixes a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants