jayzhan211 commented on PR #24648:
URL: https://github.com/apache/datafusion/pull/24648#issuecomment-5441235067
Thanks for this — a 0-row file shouldn't invalidate the ordering of the
other files, and the mixed case works nicely.
One thing to fix before merge. `new_from_files` is used for two things: the
ordering reasoning, *and* rebuilding the list of files to scan (the callers do
`flattened_files[statistics.file_index(idx)]`). Skipping empty files is right
for the first, but it means they no longer have a statistics row — so they
never make it back into the rebuilt list and silently disappear from the plan.
That's harmless when they'd contribute no rows anyway, but if *all* the
files reaching the split are empty, the rebuilt list is empty too and we get
`file_groups={0 groups: []}` → `UnknownPartitioning(0)`. Plain scans still
return the right answer, but anything with a distribution requirement then
fails the plan sanity check.
It doesn't need a fully empty table — partition filters are applied before
the split, so one empty partition is enough:
```sql
-- t partitioned by p: p=A -> 3 rows, p=B -> a single 0-row parquet file
SELECT int_col, count(*) FROM t GROUP BY int_col; -- ok: 1,
2, 3
SELECT int_col, count(*) FROM t WHERE p = 'B' GROUP BY int_col; -- fails on
this PR
```
```
SanityCheckPlan: ... does not satisfy distribution requirements:
KeyPartitioned[[int_col@0]]). Child-0 output partitioning:
UnknownPartitioning(0)
```
Same for a hash join or a window function over that filter; all of them pass
on the base commit. It only shows up with `split_file_groups_by_statistics =
true`, so it isn't a default-path break, but 0-row parquet files are common
enough in practice (DataFusion's own `COPY (SELECT ... WHERE FALSE)` writes
one, and Spark/Flink jobs emit them for empty batches) that I think it's worth
handling.
Could the empty files stay in the file list, and be excluded only from the
min/max reasoning? Appending them at the end of a group is safe since they
contribute no rows. One option that keeps `MinMaxStatistics` 1:1 with its input:
```rust
let (files, empty_files): (Vec<_>, Vec<_>) =
group.iter().partition(|f| !is_known_empty(f));
let statistics = MinMaxStatistics::new_from_files(...,
files.iter().copied())?;
sorted_indices
.iter()
.map(|(idx, _)| files[*idx].clone())
.chain(empty_files.iter().map(|f| (*f).clone()))
.collect()
```
That would also let `file_index()` / `file_count()` go away. Three call
sites rebuild groups this way: `split_groups_by_statistics`,
`split_groups_by_statistics_with_target_partitions`, and
`sort_files_within_groups_by_statistics` — the last one can currently also
produce a group with zero files (two empty files in one group), and its doc
comment still says group composition is unchanged.
A couple of smaller things:
- Worth a unit test on the rebuild path — nothing in
`test_split_groups_by_statistics*` passes an empty file today, so a regression
there wouldn't be caught by anything except the slt.
- `num_rows == Precision::Exact(0)` now appears in three files; a shared
`is_known_empty(&Statistics) -> bool` would keep the definition in one place.
- `new_from_files` on an empty input used to return a `plan_err`; it now
returns `Ok` with `is_sorted() == true`. No caller hits it today, but it's a
silent contract change.
Everything else checks out on my end: `parquet_sorted_statistics.slt` and
`parquet_statistics.slt` pass, both crates' lib tests pass, and fmt/clippy are
clean.
--
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]
