[alamb]

Which issue does this PR close?

• split out of docs: Improve documentation for SortKeyWalk #27234.

Rationale for this change

While reviewing #27191 I was researching how EquivalenceProperties tracks orderings, equal expressions, constants and groupings, and wanted to write down what I learned so others do not have to repeat it.

The concepts were documented in pieces across several structs, with no single place that explained them together.

What changes are included in this PR?

Rustdoc only. The EquivalenceProperties doc in datafusion/physical-expr/src/equivalence/properties/mod.rs becomes the hub that the other struct docs link to. It keeps the existing examples and adds:

What is the testing strategy for this PR?

CI

Are there any user-facing changes?

No.

[mhilton]

LGTM thanks. I expect I will be referring to this in the future.

[alamb]

I spent quite a while working through these examples and text, trying to make it easer to understand (aka this is not LLM spew)

[jayzhan211]

@alamb thanks for this write-up! Two of the examples describe derivations the code doesn't do today (the CAST example and the Groupings row), details and suggestions inline. The set-monotonic NULL note is non-blocking.

[jayzhan211]

While checking this section I found that the NULL placement it describes gives wrong results on main when x has NULLs: MAX(x) OVER (ORDER BY ts) is NULL until the first non-NULL value, so the output is ASC NULLS FIRST, not NULLS LAST, yet the sort is removed and the NULL row comes out first. MIN and the shrinking-frame case are wrong the same way. Fine to keep this PR as is; could the text say "when x has no NULLs" until the code is fixed?

Repro (sort removed, NULL row returned first)

set datafusion.execution.target_partitions = 1;
CREATE TABLE t(ts INT, x INT) AS VALUES (1, NULL), (2, 3), (3, 1), (4, 5);
SELECT ts, m FROM (SELECT ts, MAX(x) OVER (ORDER BY ts) AS m FROM (SELECT * FROM t ORDER BY ts))
ORDER BY m ASC NULLS LAST;
-- actual: (1,NULL),(2,3),(3,3),(4,5); expected: NULL row last

[alamb]

This is a great find -- I filed a ticket to track:

• Wrong results: ORDER BY ... NULLS LAST on a running MAX window function puts the NULL row first #27267

Fine to keep this PR as is; could the text say "when x has no NULLs" until the code is fixed?

I think that would be sort of confusing as it would be without context. Instead I removed the description of nulls first / nulls last.

Changes addressed. Waiting on next review

[alamb]

Thank you @jayzhan211 for the review. I think I have addressed all your comments

[asolimando]

After working on #27113 I wonder if functional deps shouldn't be added to EquivalenceProperties, at the end they influence properties like ordering too. WDYT?

[alamb]

It sounds like a great thing to add as a follow up PR. Thank you

[jayzhan211]

Thanks @alamb !