Skip to content

fix: consider nulls_first when propagating ordered SortProperties - #24206

Open
Nagato-Yuzuru wants to merge 3 commits into
apache:mainfrom
Nagato-Yuzuru:issue-11596
Open

fix: consider nulls_first when propagating ordered SortProperties#24206
Nagato-Yuzuru wants to merge 3 commits into
apache:mainfrom
Nagato-Yuzuru:issue-11596

Conversation

@Nagato-Yuzuru

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Rationale for this change

In the original implementation, cases where null_first is not same were not correctly handled and were still treated as ordered.

Example: a sorted ASC NULLS FIRST, b sorted ASC NULLS LAST:

a b a + b
NULL 1 NULL
1 2 3
2 4 6
3 NULL NULL

Since a + b is NULL wherever either input is, the result has nulls at both ends. The previous code returned Ordered(ASC, nulls_first: true) for this case; it now returns Unordered.

What changes are included in this PR?

Unit Tests were added and issues were fixed. Additional processing was performed on and_or (see #11596 (comment) )

SortProperties::{add, sub, gt_or_gteq, and_or} now propagate an Ordered result only when both operands agree on nulls_first; otherwise they return Unordered.

This is conservative. If one input were known to be non-null the old result could be valid. But SortProperties carries no nullability information, so Unordered is the only sound answer at this layer.

Are these changes tested?

Yes.

Are there any user-facing changes?

No.

@github-actions github-actions Bot added the logical-expr Logical plan and expressions label Aug 9, 2026
@Nagato-Yuzuru
Nagato-Yuzuru marked this pull request as ready for review August 9, 2026 18:19
@Nagato-Yuzuru Nagato-Yuzuru changed the title Fixed where binary expressions were passed in the wrong order when the null directions differed fix: consider nulls_first when propagating ordered SortProperties Aug 9, 2026
@Nagato-Yuzuru

Copy link
Copy Markdown
Contributor Author

The old query plan can lead to this sort error

In datafusion-cli

DataFusion CLI v54.1.0

>set datafusion.execution.target_partitions = 1;

-- Materialize the data through SQL so this repro is self-contained.
COPY (SELECT * FROM (VALUES
  (1,                 CAST(NULL AS INT)),
  (2,                 10),
  (3,                 1),
  (CAST(NULL AS INT), 0)
) AS t(inc_col, desc_col))
TO '/tmp/nulls_demo.csv' STORED AS CSV;

CREATE EXTERNAL TABLE demo (
  inc_col INTEGER,
  desc_col INTEGER
)
STORED AS CSV
WITH ORDER (inc_col ASC)     -- ASC  defaults to NULLS LAST  -> nulls at the end
WITH ORDER (desc_col DESC)   -- DESC defaults to NULLS FIRST -> nulls at the start
LOCATION '/tmp/nulls_demo.csv'
OPTIONS ('format.has_header' 'true');

-- Both columns really do match their declared orderings.
SELECT * FROM demo;

SELECT CAST((inc_col > desc_col) AS integer) AS c FROM demo ORDER BY c;

Get

+---------+----------+
| inc_col | desc_col |
+---------+----------+
| 1       | NULL     |   inc_col:  1, 2, 3, NULL  -> ASC NULLS LAST  ✓
| 2       | 10       |   desc_col: NULL, 10, 1, 0 -> DESC NULLS FIRST ✓
| 3       | 1        |
| NULL    | 0        |
+---------+----------+


+------+
| c    |
+------+
| NULL |
| 0    |
| 1    |
| NULL |
+------+

Order C was silently violated

@github-actions github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 9, 2026
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.05%. Comparing base (eec8b94) to head (07d0770).
⚠️ Report is 12 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff            @@
##             main   #24206    +/-   ##
========================================
  Coverage   81.05%   81.05%            
========================================
  Files        1106     1106            
  Lines      382287   382097   -190     
  Branches   382287   382097   -190     
========================================
- Hits       309851   309723   -128     
+ Misses      54121    54090    -31     
+ Partials    18315    18284    -31     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

OPTIONS ('format.has_header' 'true');

query TT
EXPLAIN SELECT CAST((inc_col>desc_col) as integer) as c from annotated_data_finite_nulls_last order by c;

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.

Do we have sqltest like the example in the description covered, like null + numeric -> null, then we expect to see the null first -> unordered

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

logical-expr Logical plan and expressions sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Propagation of ordered SortProperties should consider nulls_first

3 participants