kosiew commented on code in PR #26434:
URL: https://github.com/apache/datafusion/pull/26434#discussion_r4311580362
##########
datafusion/datasource/src/file_scan_config/mod.rs:
##########
@@ -3036,6 +3032,34 @@ mod tests {
}
}
+ /// A scan that advertises an output ordering never shares files between
+ /// partitions, even if `preserve_order` is cleared after it is built.
+ #[test]
+ fn output_ordering_keeps_files_in_order() {
+ let schema =
+ Arc::new(Schema::new(vec![Field::new("a", DataType::Float64,
false)]));
+ let sort_expr =
PhysicalSortExpr::new_default(Arc::new(Column::new("a", 0)));
+ let mut config = FileScanConfigBuilder::new(
+ ObjectStoreUrl::local_filesystem(),
+ Arc::new(MockSource::new(schema)),
+ )
+ .with_file_groups(vec![
+ FileGroup::new(vec![make_file_with_stats("file1", 0.0, 9.0)]),
+ FileGroup::new(vec![make_file_with_stats("file2", 10.0, 19.0)]),
+ ])
+ .with_output_ordering(vec![LexOrdering::new(vec![sort_expr]).unwrap()])
+ .build();
+ let options = ConfigOptions::default();
+
+ config.preserve_order = false;
+ assert!(config.preserves_file_order());
+ assert!(config.create_sibling_state(&options).is_none());
+
+ config.output_ordering = vec![];
Review Comment:
Could we also test the explicit `preserve_order = true` case with an empty
`output_ordering`? Please check that `preserves_file_order()` returns true and
`create_sibling_state(&options)` returns `None`, so we also cover the override
documented in the migration guide. This is optional.
##########
datafusion/sqllogictest/test_files/limit_pruning.slt:
##########
@@ -128,6 +128,103 @@ drop table tracking_data;
statement ok
reset datafusion.explain.analyze_level;
+# A scan that advertises an ordering must keep it when the fetch of a
+# SortPreservingMergeExec is pushed into it: no limit pruning, no work
stealing.
+statement ok
+set datafusion.execution.target_partitions = 2;
+
+statement ok
+set datafusion.explain.physical_plan_only = true;
+
+# Row group 0 of a.parquet partially matches `passes = 1` (id 1 fails), row
+# group 1 fully matches. Limit pruning would skip row group 0 and return 4, 5,
6.
+query I
+COPY (SELECT value AS id, CASE WHEN value = 1 THEN 0 ELSE 1 END AS passes FROM
generate_series(1, 6))
+TO 'test_files/scratch/limit_pruning/ordered_prune/a.parquet'
+STORED AS PARQUET OPTIONS ('format.max_row_group_size' '3');
+----
+6
+
+query I
+COPY (SELECT value AS id, 1 AS passes FROM generate_series(101, 103))
+TO 'test_files/scratch/limit_pruning/ordered_prune/b.parquet'
+STORED AS PARQUET OPTIONS ('format.max_row_group_size' '3');
+----
+3
+
+statement ok
+CREATE EXTERNAL TABLE ordered_prune STORED AS PARQUET
+LOCATION 'test_files/scratch/limit_pruning/ordered_prune/'
+WITH ORDER (id ASC);
+
+query TT
+explain select id from ordered_prune where passes = 1 order by id limit 3;
+----
+physical_plan
+01)SortPreservingMergeExec: [id@0 ASC NULLS LAST], fetch=3
+02)--DataSourceExec: <slt:ignore>limit=3, output_ordering=[id@0 ASC NULLS
LAST], file_type=parquet<slt:ignore>
+
+query I
+select id from ordered_prune where passes = 1 order by id limit 3;
+----
+2
+3
+4
+
+statement ok
+drop table ordered_prune;
+
+# File groups are [a, b] and [c], each sorted, so the scan advertises `a ASC`.
+# c.parquet holds the smallest values. If the two partitions shared one file
+# queue (a, b, c), no partition would start with c and the limit would return
0..4.
+query I
+COPY (SELECT value AS a FROM generate_series(0, 9))
+TO 'test_files/scratch/limit_pruning/ordered_steal/a.parquet';
+----
+10
+
+query I
+COPY (SELECT value AS a FROM generate_series(10, 19))
+TO 'test_files/scratch/limit_pruning/ordered_steal/b.parquet';
+----
+10
+
+query I
+COPY (SELECT value AS a FROM generate_series(-10, -1))
+TO 'test_files/scratch/limit_pruning/ordered_steal/c.parquet';
+----
+10
+
+statement ok
+CREATE EXTERNAL TABLE ordered_steal STORED AS PARQUET
+LOCATION 'test_files/scratch/limit_pruning/ordered_steal/'
+WITH ORDER (a ASC);
+
+query TT
+explain select a from ordered_steal order by a limit 5;
+----
+physical_plan
+01)SortPreservingMergeExec: [a@0 ASC NULLS LAST], fetch=5
+02)--DataSourceExec: file_groups={2 groups:
[[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/limit_pruning/ordered_steal/a.parquet,
WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/limit_pruning/ordered_steal/b.parquet],
[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/limit_pruning/ordered_steal/c.parquet]]},
projection=[a], limit=5, output_ordering=[a@0 ASC NULLS LAST],
file_type=parquet
+
+query I
+select a from ordered_steal order by a limit 5;
Review Comment:
Could we add an OFFSET test using the existing `ordered_steal` data? `ORDER
BY a LIMIT 3 OFFSET 2` should return `-8, -7, -6`, and an EXPLAIN check could
verify that the optimizer preserves the offset and pushes a fetch of 5 into the
ordered scan. This can be a follow-up since the current tests already cover the
reported bug.
--
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]
