jayzhan211 commented on code in PR #26458:
URL: https://github.com/apache/datafusion/pull/26458#discussion_r4325653060
##########
datafusion/datasource/src/file_scan_config/mod.rs:
##########
@@ -836,6 +836,23 @@ impl DataSource for FileScanConfig {
return Ok(None);
}
+ // An ordered scan can use the size-balanced split of an unordered
+ // scan when every new group is still sorted. That split keeps the
+ // files in group order and only moves the group boundaries, so groups
+ // like `[f1, f2], [f3, f4]` with a large `f1` become
+ // `[f1 head], [f1 tail, f2], ...`.
+ if output_ordering.is_some()
+ && let Some(balanced) = self.file_source.repartitioned(
+ target_partitions,
+ repartition_file_min_size,
+ None,
+ self,
+ )?
+ && balanced.validated_output_ordering() ==
self.validated_output_ordering()
Review Comment:
This is already reachable on main without this PR: with `[1, NULL]` and `[2,
3]` in two files declared `WITH ORDER (a ASC NULLS LAST)` and
`target_partitions = 1`, both files are read in one partition, the sort is
removed, and the query returns `1, NULL, 2, 3`. #26280 (for #26278) fixes it in
`MinMaxStatistics`, which is the check this PR relies on: it puts NULLs into
the file bounds when a file may contain them, treats unknown null counts as
possibly NULL, and leaves single-file groups alone. So I'd rather build on that
than add a second check here.
I checked this PR on top of #26280. Once it merges I'll rebase and add a
unit test where the balanced split would put a file with NULLs after another
file under NULLS FIRST (it is rejected and the ordered split is used), plus
sqllogictest cases with two files for NULLS FIRST and NULLS LAST that read the
rows back, with the NULLs at the allowed end (ordering kept) and at the wrong
end (sort added).
##########
datafusion/sqllogictest/test_files/repartition_scan.slt:
##########
Review Comment:
While looking into this I found that the updated plan here was itself wrong.
The table declares `int` over files that store `bigint`, and in that case the
file statistics come back with NULL min/max values and no null count, so the
check accepted `[1.parquet (100, 200), 2.parquet (1..5)]` as one sorted group.
The results only looked right because `2.parquet:0..6` reads no rows. On top of
#26280 that split is rejected and this plan goes back to what it is on main, so
the comment is accurate again. I'll add a separate case whose files are in
value order, which shows a group spanning two files and explains when that is
allowed.
--
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]
