Skip to content

fix: do not fail when an expression's ordering cannot be derived - #26884

Merged
jayzhan211 merged 4 commits into
apache:mainfrom
jayzhan211:equivalence-keep-known-ordering
Oct 31, 2026
Merged

jayzhan211 merged 4 commits into
apache:mainfrom
jayzhan211:equivalence-keep-known-ordering

Conversation

@jayzhan211

@jayzhan211 jayzhan211 commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

sqrt, ln, log2, log10, acos, asin, acosh and atanh cannot derive an ordering from their input when the input may be outside their domain (for sqrt, a negative a), so their get_properties returns an error. Two places in EquivalenceProperties treated that error as fatal, although it only means that nothing can be derived:

  • update_properties propagated it, which also discarded the ordering the expression already had from the plan. A plan sorted on sqrt(c) then does not satisfy a requirement on sqrt(c), so on main this query fails in SanityCheckPlan:

    SELECT c, row_number() OVER (ORDER BY sqrt(c))
    FROM (SELECT c FROM t ORDER BY sqrt(c) LIMIT 5);
  • discover_new_orderings propagated it out of FilterExec's properties. On a table with DOUBLE columns a and b, ordered by (a, b), this query fails on main with Execution error: Input range of SQRT contains out-of-domain values:

    SELECT * FROM t WHERE a = sqrt(b);

What changes are included in this PR?

  • update_properties treats properties that cannot be derived from the children as unknown, with an unbounded range of the expression's type, instead of failing. The node's own known ordering is kept, and parents such as -sqrt(c) can still derive their ordering from it.
  • discover_new_orderings skips a candidate whose properties cannot be derived, so no new ordering is found from it.

Are these changes tested?

Yes. equivalence_properties.slt covers both queries above and -sqrt(c); each case fails without its change. test_known_ordering_when_expr_properties_fail checks that a known ordering on sqrt(a) satisfies only a requirement with the same sort options, and that sqrt(a) is not ordered when nothing is ordered or only a is.

Are there any user-facing changes?

Yes: the two queries above now plan and run.

@github-actions github-actions Bot added the physical-expr Changes to the physical-expr crates label Oct 29, 2026
@jayzhan211
jayzhan211 marked this pull request as ready for review October 29, 2026 12:44

@viirya viirya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @jayzhan211, the fix looks correct to me. update_properties is only reached through EquivalenceProperties::get_expr_properties, which already maps any error to ExprProperties::new_unknown() for the whole tree. This change moves that same fallback down to the failing node, so we never learn less than before. And a node only becomes Ordered when the ordering equivalence class actually contains it.

I think this fixes a user-facing failure on main, not just a check right after EnsureRequirements. ordering_satisfy_requirement always goes through get_expr_properties, so an ordering on sqrt(a) doesn't satisfy a requirement on sqrt(a). For example, on main:

SELECT a, row_number() OVER (ORDER BY sqrt(a))
FROM (SELECT a FROM t ORDER BY sqrt(a) LIMIT 10);

fails in SanityCheckPlan with does not satisfy order requirements: [sqrt(a@0) ASC NULLS LAST]. Child-0 order: [[sqrt(a@0) ASC NULLS LAST]]. With this PR it plans with a single TopK sort and returns the right rows (acos behaves the same). Could you add this, or something like it, as a sqllogictest (e.g. in window.slt) and update the rationale? An end-to-end test would protect the fix better than the unit test alone.

The same bug class, a get_properties error treated as fatal when it only means "cannot be derived", is still in discover_new_orderings:

let expr_properties = equivalent_expr.get_properties(&child_properties)?;

On a table declared WITH ORDER (s ASC, a ASC), SELECT * FROM t WHERE s = sqrt(a) fails with Execution error: Input range of SQRT contains out-of-domain values. The error goes FilterExec::compute_properties → add_equal_conditions → update_oeq_cache → discover_new_orderings, and it still fails with this PR. Skipping that candidate (let Ok(expr_properties) = ... else { continue; };) seems to match what this PR does: if nothing can be derived, no new ordering is discovered. Would you fix it here so the bug class is closed in one place, or would you rather track it in a follow-up issue?

node.data = node
.expr
.get_properties(&children_props)
.unwrap_or_else(|_| ExprProperties::new_unknown());

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Minor, optional: the fallback range here is make_unbounded(&DataType::Null), so parents see an ordered child with an untyped range and stop deriving anything. With ordering = [sqrt(a) ASC], -sqrt(a) and CAST(sqrt(a) AS DOUBLE) both come out Unordered. ScalarFunctionExpr::get_properties already recovers the output type for an unbounded Null range. Would something like this be worth doing here too?

.or_else(|_| {
    Interval::make_unbounded(&node.expr.data_type(eq_properties.schema())?)
        .map(|range| ExprProperties::new_unknown().with_range(range))
})?;

With it, both of those expressions keep their (reversed / same) ordering. An unbounded typed range is what a Column leaf gets, so it shouldn't let a parent assume any bounds. Fine to leave this for later if you'd rather keep the PR minimal.

/// negative, so its ordering function fails. A plan sorted on `sqrt(a)`
/// still satisfies a requirement on `sqrt(a)`.
#[test]
fn test_known_ordering_when_expr_properties_fail() -> Result<()> {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This pins down the root cause nicely. As mentioned in the summary, the window query on main fails SanityCheckPlan because of this same bug. A sqllogictest next to this unit test would cover the planner path that users actually hit.

@kosiew kosiew 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.

@jayzhan211,

Thanks for the fix. The reported failure is valid: an explicitly known expression ordering should satisfy an identical ordering requirement even when its properties cannot be derived from its children. The local fallback is appropriate and leaves successful inference unchanged.

The important boundary is between property inference and independently established equivalence knowledge. Recovering with unknown properties at the failing node preserves that distinction without asserting monotonicity or suppressing execution errors. This also aligns with the existing recovery behavior in get_expr_properties.

I found no additional blocking issues. The suggestions below would strengthen regression coverage but are not required for this fix. I am approving the change.

)?);
let ordering: LexOrdering =
[PhysicalSortExpr::new_default(Arc::clone(&sqrt_a)).asc()].into();
let mut eq_properties = EquivalenceProperties::new(Arc::clone(&schema));

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.

Optional: could we add negative controls using the same schema and expression? Verify that sqrt(a) ordering is not satisfied when no ordering is registered or when only a is ordered, since the unbounded input range prevents sqrt_order from deriving ordering. This would confirm that recovery does not manufacture ordering guarantees.

&schema,
Arc::new(ConfigOptions::default()),
)?);
let ordering: LexOrdering =

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.

Optional: could we test each ASC/DESC and NULLS FIRST/LAST combination using a fresh EquivalenceProperties instance? Identical requirements should succeed, while changing either option should fail. Since sqrt(a) is nullable, this would verify that recovery preserves sort options rather than merely recognizing the expression as ordered.

@jayzhan211 jayzhan211 changed the title fix: keep an expression's known ordering when its properties cannot be derived fix: do not fail when an expression's ordering cannot be derived Oct 31, 2026
@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Oct 31, 2026
@jayzhan211
jayzhan211 added this pull request to the merge queue Oct 31, 2026
@jayzhan211

Copy link
Copy Markdown
Contributor Author

Thanks @viirya @kosiew !

Merged via the queue into apache:main with commit 69a7fef Oct 31, 2026
44 of 45 checks passed
@jayzhan211
jayzhan211 deleted the equivalence-keep-known-ordering branch October 31, 2026 02:42
dwsmith1983 pushed a commit to dwsmith1983/datafusion that referenced this pull request Oct 31, 2026
… constants (apache#26853)

## Which issue does this PR close?

- None. This is a refactor of how `EquivalenceProperties` checks sort
requirements. It also makes the planning that
depends on hash map order, described below, deterministic; there is no
issue for that.

## Rationale for this change

The planner removes a sort when it can prove that its input is already
sorted the required way. For a
requirement like `[a, b]`, it first checks `a`. Then, since `b` only has
to be sorted among rows that tie on `a`,
it treats `a` as a constant: it clones the `EquivalenceProperties`, adds
`a` with `add_constants`, and checks `b`
against the clone.

This has three costs:

- **Planning time.** Every check with more than one key, and every
`find_longest_permutation` call, clones the
properties. Without the clone, checks with four keys are 2-4x faster and
`find_longest_permutation` 2-18x
  (benchmarks below).
- **Plans depend on hash map order.** The clone finds orderings that
follow from the satisfied key through a side
effect of `add_constants` (it strips constants from the stored orderings
and re-runs ordering discovery once,
over the leading expressions in hash map order). For example, with
ordering `[k, x, y, z]`,
`x = CAST(y AS BIGINT)` and `y = CAST(z AS INT)`, the requirement `[k,
z]` holds. `main` proves it in about half
of the runs when a second ordering `[y, w]` exists, and never without
it, so the same query can plan with or
  without a sort.
- **The trick is written three times.** `ordering_satisfy_requirement`
and `common_sort_prefix_length` are two
copies of the same loop, and `find_longest_permutation` does its own
clone plus `add_constants`. A "constant"
that only means "satisfied so far" is hard to tell apart from a real
constant when reading the code.

This PR keeps the idea, but records the satisfied keys in a separate
`SortKeyWalk`, which *pins* them, instead of changing
a copy of the `EquivalenceProperties`.

## What changes are included in this PR?

**How the walk works.** `SortKeyWalk`, in the new file
`properties/walk.rs`, checks the keys of a requirement one by one
and keeps a set of *pinned* expressions: the keys satisfied so far.
Within rows that tie on the pinned keys, a pinned
expression has a single value, so every ordering can skip it. For each
ordering, the walk keeps its *head*: its first
expression that is not fixed (a literal, a constant, pinned, or computed
only from fixed expressions). A key is
satisfied if it is fixed, if it is a head in the required direction, or,
as before, if its order follows from such
expressions (e.g. `CAST(a AS BIGINT)` when `a` is a head).

For example, with orderings `[a, b]` and `[b, c]` and the requirement
`[a, b, c]`: `a` heads `[a, b]`, so it is
satisfied and pinned. `b` now heads both orderings, so it is satisfied
and pinned. `[a, b]` is used up, `[b, c]` now
has head `c`, and `c` is satisfied.

Pinning a key also pins its equivalence class and, as before, the
arguments it determines (`a` for
`CAST(a AS BIGINT)`). After each pin, the walk adds the orderings that
the new heads imply (the rule of
`discover_new_orderings`), until none is new. The walk never changes the
`EquivalenceProperties` it reads.

**Commits.**

1. Benchmark cases in
`datafusion/physical-expr/benches/equivalence_properties.rs` for
`find_longest_permutation`,
window-shaped orderings and a deeply nested sort key, so they can run on
`main` too.
2. The walk:
- `ordering_satisfy_requirement` and `common_sort_prefix_length` both
call `SortKeyWalk::sorted_prefix_len` instead of
each cloning the properties and adding constants.
`find_longest_permutation` pins found expressions in a
`SortKeyWalk` instead of cloning (and skips pinning once every
expression is found). `get_expr_properties` reads
     through a `SortKeyWalk` with nothing pinned.
- One rule is new: a key is also satisfied if *any* head is that key in
the required direction. `main` looked
only at the first ordering the key leads (see the plan differences).
- Removed `add_satisfied_key_constants` (its job is now
`SortKeyWalk::pin_key`). The free function `update_properties`
is now a method of `SortKeyWalk`. The per-ordering check in
`discover_new_orderings` is now `implied_ordering`, and
the body of `OrderingEquivalenceClass::is_expr_partial_const` is now
`is_partial_const`; the walk shares both.
- `OrderingEquivalenceClass::get_options` and `is_expr_partial_const`
are no longer called inside DataFusion;
     they stay as public API.
- The walk follows two rules that landed on `main` while this PR was
open: properties that cannot be derived
are unknown rather than an error (apache#26884), and a volatile expression is
never fixed, while a leaf that reports
itself as a singleton, such as an uncorrelated scalar subquery, is
(apache#26836).
3. `is_partial_const` now says what it checks: the expression leads one
ordering in some direction and another in
the exact reverse (`main` removed `(descending, nulls_first)` tuples
from two hash sets). Its doc says why this
exists: a whole-partition window aggregate such as `SUM(x) OVER
(PARTITION BY k)` records that it is constant
   per `k` by adding `[k, w]` in all four directions of `w`.
4. Review follow-ups: `Walk` is renamed `SortKeyWalk`, and
`find_longest_permutation` pins what it finds in normal
form (a new row under plan differences). New unit tests, and the
benchmarks check each answer before timing it.
5. Randomized tests in `datafusion/core/tests/fuzz_cases/equivalence`
with cast equalities and whole-partition window
   orderings (see testing).
6. A unit test for the walk's rules from apache#26836.

**Where to start reading:** the `SortKeyWalk` doc comment, then
`SortKeyWalk::sorted_prefix_len` and
`SortKeyWalk::pin`.

**Risk:** if the walk claims an ordering the data does not have, a
needed sort is removed and results are wrong.
The randomized tests compare every answer with real rows to test exactly
this.

### Plan differences

No plan changes in the existing tests (see testing). On hand-built
inputs, this PR proves an ordering where
`main` does not:

| Input | `main` | This PR |
|---|---|---|
| The chain of casts above | satisfied in some runs (hash map order) |
satisfied |
| Column `x` is the constant `5`, ordering `[k, b % 5]`,
`find_longest_permutation([5, k, b % 5])` | `5, k`: adding the literal
`5` as a constant puts it into `x`'s class, so `b % 5` normalizes to `b
% x` and no longer matches | `5, k, b % 5` |
| `b = c`, ordering `[a + b, d]`, `find_longest_permutation([a + c, d])`
| `a + c`: it pins `a + c` as given, which does not match the stored `a
+ b` | `a + c, d` |
| `a` leads orderings in two directions that are not opposite, e.g. `[a
DESC NULLS FIRST, k, c]`, `[k, a DESC NULLS FIRST, c]` and `[k, a ASC
NULLS FIRST]`; requirement `[k, a ASC NULLS FIRST]` | satisfied in 4 of
the 6 declaration orders | satisfied in all 6 |

Each extra ordering really holds, so these can only remove sorts.

One case is worse: the same orderings with a requirement on an
expression *computed from* `a`,
`[k, CAST(a AS BIGINT) ASC NULLS FIRST]`. For a computed expression,
both versions give `a` one direction, from the
first ordering `a` leads. `main` looks at its clone, where removing `k`
makes the first two orderings duplicates and
deduplication reorders the rest; this PR looks at the orderings as
stored. `main` proves it in 4 of the 6
declaration orders and this PR in 2, so in 2 of them a sort is kept that
`main` removes. This PR never proves it
where `main` does not.

## What is the testing strategy for this PR?

- New unit tests in `walk.rs`: an ordering found only after pinning
(`ordering_found_after_pinning`), the chain of
casts above (`chained_orderings_found_after_pinning`, fails on `main`),
and the two-direction case
(`any_head_in_required_direction`, which fails without the any-head
check above; `main` passes it), the partial
constant check of a computed key after a pin
(`partial_constant_after_pinning`, fails without that check), and the
two `find_longest_permutation` rows above
(`find_longest_permutation_with_literal` fails on `main`,
`find_longest_permutation_pins_normal_form` fails without the normal
form), and volatile functions and scalar
subqueries (`volatile_function_and_scalar_subquery_keys`, fails without
either rule from apache#26836). The rule from
apache#26884 is covered by its own tests, which now run through the walk. New
unit test `test_is_expr_partial_const`
  in `ordering.rs`.
- New randomized tests in `datafusion/core/tests/fuzz_cases/equivalence`
(run with `--features extended_tests`):
`test_ordering_satisfy_with_casts_and_windows_random` and
`test_find_longest_permutation_with_casts_and_windows_random`.
They build sorted tables and declare only properties that hold on them:
a lossy and a lossless cast equality,
`[k, w]` in all four directions of a `w` that has one value per `k`, a
second independent ordering and a constant.
Every ordering claimed or found must hold on the rows, and orderings
that follow from the declared properties
must be proven. They also pass on `main`. Removing the walk's partial
constant check, or the orderings it adds
after a pin, makes them fail, while the existing equivalence fuzz tests
still pass. They take 1-2 seconds each in
  a debug build.
- All existing tests pass unchanged.
- Checked locally, not part of this PR:
- Side by side: with the old algorithms kept next to the walk, every
call to the four entry points (`ordering_satisfy_requirement`,
`common_sort_prefix_length`, `find_longest_permutation`,
`get_expr_properties`) ran both and logged any difference. The only
difference was the chain-of-casts unit test above, over the
`datafusion-physical-expr`, `datafusion-physical-plan` and
`datafusion-physical-optimizer` unit tests,
`core_integration`, all sqllogictests, and the equivalence fuzz tests
(`--features extended_tests`).
- Against data, at a larger scale than the randomized tests above: small
sorted tables with only true properties,
every answer of the four entry points checked against the rows. Over 9.6
million cases, neither version
claimed an ordering the rows do not have, and both returned the same
errors. The cases where `main` and this
    PR differ are the ones listed above.
- The old and new `is_partial_const` agree on every sequence of up to 6
sort directions over two expressions
    (299,593 sequences).

### Benchmarks

From the benchmark bot on this PR (`c4a-highmem-16`, Neoverse-V2).
`equivalence_properties` compares this PR
with its first commit, which is `main` plus the new benchmark cases.
Time of this PR / time of `main`, below 1 is
faster. Columns are the number of equivalence classes.

| Check | 2 | 8 | 32 |
|---|---|---|---|
| `ordering_satisfy`, 1 key | 0.85 | 0.85 | 0.85 |
| `ordering_satisfy`, 1 key, unsatisfied | 0.87 | 0.87 | 0.87 |
| `ordering_satisfy`, 4 keys | 0.42 | 0.38 | 0.28 |
| `ordering_satisfy_requirement`, 1 key | 0.78 | 0.77 | 0.77 |
| `ordering_satisfy_requirement`, 4 keys | 0.39 | 0.36 | 0.26 |
| `extract_common_sort_prefix`, 4 keys | 0.42 | 0.38 | 0.28 |

Other shapes:

| Case | Ratio |
|---|---|
| `find_longest_permutation`, 1 expression (2 / 32 classes) | 0.12 /
0.06 |
| `find_longest_permutation`, 5 expressions, one found per round (2 / 32
classes) | 0.48 / 0.35 |
| `W` window aggregates over `PARTITION BY k`, i.e. `[k, w_i]` in four
directions per window; requirement `[k, w_1]` (`W` = 4 / 16 / 32) | 0.16
/ 0.08 / 0.06 |
| Requirement `[c0, e, c1]` with `e = c1 + 1 + ... + 1` nested `d` deep
(`d` = 4 / 8 / 16 / 32); `+` may overflow, so `e` is unordered and both
versions stop at `e` | 0.79 / 0.99 / 1.13 / 1.28 |

The last row is the one shape that is slower. For every node of `e`,
both versions check whether it is constant by
looking each sub-expression up in a hash map, which hashes the whole
sub-expression, so the cost grows roughly with
the cube of the depth. This PR also looks each one up in its set of
pinned expressions, which `main` does not have.
Sort keys nested this deep are rare: up to depth 8 this PR is as fast or
faster. Measured locally (laptop, where
the ratios at depth 16 and 32 run a bit higher than on the bot), a
right-nested key `1 + (1 + (... + c1))` and a `+`
with overflow checks, where `e` is ordered and the check goes on to
`c1`, give the same ratios as this row.

After rebasing on apache#26884 and apache#26836, a local run of
`equivalence_properties` against the new `main` (laptop, 5 rounds)
gives the same picture: 0.31 to 0.88 for the checks in the first table,
0.08 to 0.49 for the other shapes, and
0.82 / 1.00 / 1.22 / 1.45 for the deep key.

Planning time end to end (`sql_planner`, this PR / `main`): TPC-H 1.00,
TPC-DS 1.00, ClickBench 1.01. The physical
planning cases range from 0.95 (sorted unions of Int64 inputs) to 1.03.
The logical planning cases, which run no code
this PR changes, range from 0.97 to 1.05, which is the noise of this
runner.

## Are there any user-facing changes?

No public API change. Plans differ only in the cases listed above.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

physical-expr Changes to the physical-expr crates sqllogictest SQL Logic Tests (.slt) v56.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants