Nagato-Yuzuru commented on code in PR #24206:
URL: https://github.com/apache/datafusion/pull/24206#discussion_r3757710408
##########
datafusion/sqllogictest/test_files/order.slt:
##########
@@ -1507,9 +1509,36 @@ logical_plan
03)----TableScan: annotated_data_finite projection=[inc_col, desc_col]
physical_plan
01)SortPreservingMergeExec: [c@0 ASC NULLS LAST]
+02)--SortExec: expr=[c@0 ASC NULLS LAST], preserve_partitioning=[true]
+03)----ProjectionExec: expr=[CAST(inc_col@0 > desc_col@1 AS Int32) as c]
+04)------RepartitionExec: partitioning=RoundRobinBatch(2), input_partitions=1,
maintains_sort_order=true
+05)--------DataSourceExec: file_groups={1 group:
[[WORKSPACE_ROOT/datafusion/core/tests/data/window_1.csv]]},
projection=[inc_col, desc_col], output_orderings=[[inc_col@0 ASC NULLS LAST],
[desc_col@1 DESC]], file_type=csv, has_header=true
+
+# With matching null placement the comparison keeps its order: no sort needed.
+statement ok
+CREATE EXTERNAL TABLE annotated_data_finite_nulls_last (
+ ts INTEGER,
+ inc_col INTEGER,
+ desc_col INTEGER,
+)
+STORED AS CSV
+WITH ORDER (inc_col ASC NULLS LAST)
+WITH ORDER (desc_col DESC NULLS LAST)
+LOCATION '../core/tests/data/window_1.csv'
+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;
Review Comment:
Thanks for comment. The add case is added.
i found that the + slt would produce an identical plan with and without this
patch, and wouldn't guard the change. The add/sub half of the fix is only
observable in the unit tests; gt_or_gteq and and_or are the parts reachable end
to end.
#23910 made `BinaryExpr::arithmetic_sort_properties` fall back to Unordered
unless it can prove the arithmetic doesn't overflow.
```rust
let cannot_overflow = !range.is_unbounded()
&& !unsigned_subtraction_may_underflow(self.op, l_range,
r_range, range);
if !wraps_in_domain && (self.fail_on_overflow || cannot_overflow) {
sort_properties
} else {
SortProperties::Unordered
}
```
[Here](https://github.com/apache/datafusion/pull/23910/changes#diff-32f7f18dcd86a268e7e1e0134eae6ae002bd42e61180cfabd60944566b10f6d8R147-R153)
if endpoint null, so range.is_unbounded() holds for any col + col and the
result is Unordered regardless of null placement. SortProperties::add's return
value is discarded before nulls_first can matter. The SortExec still survives
if declaring both columns ASC NULLS FIRST.
Before current patch:
```sql
DataFusion CLI v54.1.0
> set datafusion.execution.target_partitions = 1;
0 row(s) fetched.
Elapsed 0.001 seconds.
> COPY (VALUES (NULL, 1), (1, 2), (2, 4), (3, NULL))
TO 'test_files/scratch/order/mixed_null_placement.csv'
OPTIONS ('format.has_header' 'true');
+-------+
| count |
+-------+
| 4 |
+-------+
1 row(s) fetched.
Elapsed 0.002 seconds.
> CREATE EXTERNAL TABLE mixed_null_placement (
a BIGINT,
b BIGINT
)
STORED AS CSV
WITH ORDER (a ASC NULLS FIRST)
WITH ORDER (b ASC NULLS LAST)
LOCATION 'test_files/scratch/order/mixed_null_placement.csv'
OPTIONS ('format.has_header' 'true');
0 row(s) fetched.
Elapsed 0.001 seconds.
> SELECT a, b, a + b AS s FROM mixed_null_placement ORDER BY s ASC NULLS
FIRST;
+------+------+------+
| a | b | s |
+------+------+------+
| NULL | 1 | NULL |
| 3 | NULL | NULL |
| 1 | 2 | 3 |
| 2 | 4 | 6 |
+------+------+------+
4 row(s) fetched.
Elapsed 0.003 seconds.
> EXPLAIN SELECT a + b AS s FROM mixed_null_placement ORDER BY s;
+---------------+-------------------------------+
| plan_type | plan |
+---------------+-------------------------------+
| physical_plan | ┌───────────────────────────┐ |
| | │ SortExec │ |
| | │ -------------------- │ |
| | │ s@0 ASC NULLS LAST │ |
| | └─────────────┬─────────────┘ |
| | ┌─────────────┴─────────────┐ |
| | │ DataSourceExec │ |
| | │ -------------------- │ |
| | │ files: 1 │ |
| | │ format: csv │ |
| | └───────────────────────────┘ |
| | |
+---------------+-------------------------------+
1 row(s) fetched.
Elapsed 0.001 seconds.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
