You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "jayzhan211 (via GitHub)" <gi...@apache.org> on 2026/10/16 22:14:50 UTC
[PR] fix: keep file order for scans that advertise an output ordering [datafusion]
jayzhan211 opened a new pull request, #26434:
URL: https://github.com/apache/datafusion/pull/26434
## Which issue does this PR close?
- Closes #24272.
- Makes #24215 moot (the limit nodes no longer carry an ordering to lose).
## Rationale for this change
`ORDER BY ... LIMIT` over a parquet table declared `WITH ORDER` can return the wrong rows with default settings:
```sql
-- 8 sorted files plus one backfill file holding the smallest values
SET datafusion.execution.target_partitions = 8;
CREATE EXTERNAL TABLE t STORED AS PARQUET WITH ORDER (a ASC) LOCATION '...';
SELECT a FROM t ORDER BY a LIMIT 3; -- returns 0, 1, 2; min(a) is -100000
```
The plan is `SortPreservingMergeExec: fetch=3 -> DataSourceExec: output_ordering=[a ASC], limit=3`.
`LimitPushdown` pushes the merge's fetch into the scan and, because its own `preserve_order` flag defaults to `false`,
calls `with_preserve_order(false)`. That overwrites the `true` the scan was built with, which:
- turns on work stealing: the partitions share one file queue, so a partition can read a higher file before a lower one
and its stream is no longer sorted (the wrong rows above);
- turns on parquet limit pruning: the scan may skip a partially matching row group for a later fully matching one (#24272).
Whether a scan must keep its file order is passed through six places today: `EnforceSorting` sets `required_ordering`
on limit nodes, `LimitPushdown` copies it into a bool, `ExecutionPlan::with_preserve_order` forwards it through
projection/filter/coalesce/merge, `DataSource::with_preserve_order` sets `FileScanConfig::preserve_order`, and the file
stream and parquet opener read it. Any place that drops it means "reorder freely" (#24173, #24215, #24272).
The scan already knows the answer: it advertises its output ordering, and every parent that depends on order relies on
exactly that.
## What changes are included in this PR?
- `FileScanConfig::preserves_file_order()`: a scan with an output ordering always reads its files in order. Work
stealing and parquet limit pruning use only this.
- `LimitPushdown` no longer tracks or pushes order sensitivity.
- Deprecated, no longer used: `ExecutionPlan::with_preserve_order`, `DataSource::with_preserve_order` (the 5 forwarding
overrides are removed), and `GlobalLimitExec`/`LocalLimitExec::required_ordering` / `set_required_ordering`
(the field is removed; the protobuf field is ignored on read and no longer written).
- Upgrade guide entry.
## What is the testing strategy for this PR?
- `limit_pruning.slt`: two new cases, both fail on `main` (`0..4` instead of `-10..-6`; `4, 5, 6` instead of `2, 3, 4`).
- Unit test `output_ordering_keeps_files_in_order` in `file_scan_config`.
- Updated tests that asserted the removed plumbing (core physical optimizer, proto).
Benchmark: TPC-H SF10 `lineitem` declared `WITH ORDER (l_orderkey)`, release-nonlto, same commit with and without
this change, median of 10 runs over two interleaved rounds (ms):
| Query | pushdown_filters | main | this PR |
|---|---|---|---|
| Q1 `ORDER BY l_orderkey LIMIT 10` | off / on | 19.5 / 19.0 | 19.0 / 19.0 |
| Q2 `WHERE l_quantity > 49 ORDER BY l_orderkey LIMIT 10` | off / on | 28.5 / 20.0 | 28.0 / 20.0 |
| Q3 `WHERE l_shipdate > '1998-11-25' ORDER BY l_orderkey LIMIT 10` | off / on | 35.0 / 29.0 | 37.5 / 29.0 |
| Q4 `WHERE l_quantity > 49 LIMIT 10` | off / on | 18.0 / 17.5 | 17.0 / 17.5 |
| Q5 `WHERE l_shipdate > '1998-11-25' LIMIT 10` | off / on | 27.5 / 23.5 | 26.5 / 22.5 |
A rerun of Q3 (off) with 15 interleaved runs each gave 37-42 ms on main and 36-41 ms with this PR. Scans with a declared ordering
already read files in order whenever no limit is pushed into them (the builder sets `preserve_order`), so this PR only
removes the reordering on the path where a limit is pushed in; tables without a declared ordering are unaffected.
## Are there any user-facing changes?
- Wrong results fixed for `ORDER BY ... LIMIT` over declared-ordering file scans.
- A plain `LIMIT` (no `ORDER BY`) over a declared-ordering table now keeps file order, so it no longer uses work stealing
or limit pruning. Tables without a declared ordering are unchanged.
- API deprecations listed above (`api change` label).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep file order for scans that advertise an output ordering [datafusion]
Posted by "adriangb (via GitHub)" <gi...@apache.org>.
adriangb commented on code in PR #26434:
URL: https://github.com/apache/datafusion/pull/26434#discussion_r4300698464
##########
docs/source/library-user-guide/upgrading/56.0.0.md:
##########
@@ -1183,3 +1183,30 @@ A registered plan needs no `PhysicalExtensionCodec` at all, and two crates' plan
`PhysicalExtensionCodec` is not deprecated: extension `PhysicalExpr`s and UDF payloads still need it, and it remains the fallback for unmigrated plans.
See [issue #24625](https://github.com/apache/datafusion/issues/24625) for details.
+
+### Order-preservation hooks for limit pushdown are deprecated
+
+A file scan that advertises an output ordering now always reads its files in
+order, so `ORDER BY ... LIMIT` queries no longer depend on `LimitPushdown`
+telling the scan to keep its order. Before this change, pushing the fetch of a
+`SortPreservingMergeExec` into an ordered Parquet scan could return the wrong
+rows, because the scan then shared files between partitions or skipped row
+groups based on its limit.
+
+The plumbing that carried this request is deprecated and no longer used:
+
+- `ExecutionPlan::with_preserve_order` and `DataSource::with_preserve_order`
+ are never called. Built-in operators no longer override them.
Review Comment:
Custom `DataSource` and `ExecutionPlan` implementations that override `with_preserve_order` do not get a compile error or a warning: `#[deprecated]` on a trait method warns callers only, not implementors. After this change, DataFusion never calls their override, so their behaviour changes silently.
Please add to this entry:
- Overrides of these methods are now dead code.
- A custom scan that advertises an output ordering must keep that order by itself, also when a limit is pushed into it.
##########
datafusion/datasource/src/file_scan_config/mod.rs:
##########
@@ -1220,7 +1206,7 @@ impl DataSource for FileScanConfig {
&self,
config: &ConfigOptions,
) -> Option<Arc<dyn Any + Send + Sync>> {
- if self.preserve_order
+ if self.preserves_file_order()
Review Comment:
This also turns off work stealing for a plain `LIMIT` (no `ORDER BY`) over a declared-ordering table, and the PR does not measure skewed file groups.
I measured it: 16 sorted files, 38M rows (4 files of 8M rows and 12 of 0.5M), `WITH ORDER (a ASC)`, `target_partitions = 8`, release-nonlto, base = merge-base `81824e82`. Median of 5 interleaved runs, best of 3 per run (ms):
| Query | pushdown_filters | base | this PR |
|---|---|---|---|
| `SELECT * FROM t WHERE b > 0.999999 LIMIT 10` (skewed) | off | 13 | 25 |
| same (skewed) | on | 5 | 6 |
| same (uniform, 16 × 2M) | off | 9 | 12 |
| `SELECT * FROM t LIMIT 10` / `WHERE b > 0.99 LIMIT 10` | off / on | 3-5 | 3-5 |
So a selective filter over skewed groups is about 2x slower with `pushdown_filters = false`. The planner puts `f00` and `f01` (both 8M rows) in one group, and without work stealing that partition sets the wall time.
Please do one of these:
- Keep work stealing when no parent needs the order. That keeps the fix for `ORDER BY ... LIMIT`.
- Or state this cost in the PR description and the upgrade guide, so the trade-off is an explicit decision.
##########
datafusion/physical-plan/src/limit.rs:
##########
@@ -347,21 +344,16 @@ impl GlobalLimitExec {
input,
skip,
fetch,
- required_ordering,
+ // Deprecated, ignored.
+ required_ordering: _,
Review Comment:
Please add a test that decodes a `GlobalLimitExecNode` / `LocalLimitExecNode` with a non-empty `required_ordering` (a payload from an older writer) and checks that the decode succeeds and ignores the field. Without it, nothing guards the "ignored when read" promise in the proto comment.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep file order for scans that advertise an output ordering [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #26434:
URL: https://github.com/apache/datafusion/pull/26434#issuecomment-6267563765
Thanks @adriangb @kosiew @xudong963!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep file order for scans that advertise an output ordering [datafusion]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #26434:
URL: https://github.com/apache/datafusion/pull/26434#issuecomment-6226053553
## [Codecov](https://app.codecov.io/gh/apache/datafusion/pull/26434?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:x: Patch coverage is `87.27273%` with `7 lines` in your changes missing coverage. Please review.
:white_check_mark: Project coverage is 82.89%. Comparing base ([`81824e8`](https://app.codecov.io/gh/apache/datafusion/commit/81824e8230c1189acd2243e1e4ce7aa6e8b38e82?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`d3f7cb3`](https://app.codecov.io/gh/apache/datafusion/commit/d3f7cb3ee58c4d5d05c7f1e8915cda51082ac049?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 7 commits behind head on main.
| [Files with missing lines](https://app.codecov.io/gh/apache/datafusion/pull/26434?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Patch % | Lines |
|---|---|---|
| [datafusion/physical-plan/src/limit.rs](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&filepath=datafusion%2Fphysical-plan%2Fsrc%2Flimit.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1wbGFuL3NyYy9saW1pdC5ycw==) | 76.47% | [4 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...zer/src/ensure\_requirements/enforce\_sorting/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&filepath=datafusion%2Fphysical-optimizer%2Fsrc%2Fensure_requirements%2Fenforce_sorting%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1vcHRpbWl6ZXIvc3JjL2Vuc3VyZV9yZXF1aXJlbWVudHMvZW5mb3JjZV9zb3J0aW5nL21vZC5ycw==) | 66.66% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [...sure\_requirements/enforce\_sorting/sort\_pushdown.rs](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&filepath=datafusion%2Fphysical-optimizer%2Fsrc%2Fensure_requirements%2Fenforce_sorting%2Fsort_pushdown.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9waHlzaWNhbC1vcHRpbWl6ZXIvc3JjL2Vuc3VyZV9yZXF1aXJlbWVudHMvZW5mb3JjZV9zb3J0aW5nL3NvcnRfcHVzaGRvd24ucnM=) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
| [datafusion/proto/src/physical\_plan/mod.rs](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&filepath=datafusion%2Fproto%2Fsrc%2Fphysical_plan%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZGF0YWZ1c2lvbi9wcm90by9zcmMvcGh5c2ljYWxfcGxhbi9tb2QucnM=) | 66.66% | [0 Missing and 1 partial :warning: ](https://app.codecov.io/gh/apache/datafusion/pull/26434?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) |
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #26434 +/- ##
=======================================
Coverage 82.89% 82.89%
=======================================
Files 1153 1154 +1
Lines 457483 457503 +20
Branches 457483 457503 +20
=======================================
+ Hits 379210 379253 +43
+ Misses 55263 55245 -18
+ Partials 23010 23005 -5
```
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/datafusion/pull/26434?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep file order for scans that advertise an output ordering [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 merged PR #26434:
URL: https://github.com/apache/datafusion/pull/26434
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep file order for scans that advertise an output ordering [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on code in PR #26434:
URL: https://github.com/apache/datafusion/pull/26434#discussion_r4305428691
##########
docs/source/library-user-guide/upgrading/56.0.0.md:
##########
@@ -1183,3 +1183,30 @@ A registered plan needs no `PhysicalExtensionCodec` at all, and two crates' plan
`PhysicalExtensionCodec` is not deprecated: extension `PhysicalExpr`s and UDF payloads still need it, and it remains the fallback for unmigrated plans.
See [issue #24625](https://github.com/apache/datafusion/issues/24625) for details.
+
+### Order-preservation hooks for limit pushdown are deprecated
+
+A file scan that advertises an output ordering now always reads its files in
+order, so `ORDER BY ... LIMIT` queries no longer depend on `LimitPushdown`
+telling the scan to keep its order. Before this change, pushing the fetch of a
+`SortPreservingMergeExec` into an ordered Parquet scan could return the wrong
+rows, because the scan then shared files between partitions or skipped row
+groups based on its limit.
+
+The plumbing that carried this request is deprecated and no longer used:
+
+- `ExecutionPlan::with_preserve_order` and `DataSource::with_preserve_order`
+ are never called. Built-in operators no longer override them.
Review Comment:
Added both points: overrides are now dead code with no compiler warning, and a custom scan that advertises an output ordering must keep it by itself, also when a limit is pushed into it with `with_fetch`.
##########
datafusion/datasource/src/file_scan_config/mod.rs:
##########
@@ -1220,7 +1206,7 @@ impl DataSource for FileScanConfig {
&self,
config: &ConfigOptions,
) -> Option<Arc<dyn Any + Send + Sync>> {
- if self.preserve_order
+ if self.preserves_file_order()
Review Comment:
Thanks for measuring this. I reproduced it with the same shape (16 sorted files: f00-f03 have 8M rows, f04-f15 have 0.5M; `WITH ORDER (a ASC)`; 8 partitions). The slowdown is real but smaller on my machine: `WHERE b > 0.999999 LIMIT 10` goes from 12 to 14 ms (`pushdown_filters` off) and from 4 to 6 ms (on).
I went with your second option and documented the cost in the upgrade guide and the PR description. Measuring further showed why I'd rather not keep stealing for this one path. On `main`, a scan with a declared ordering already reads files in order for every other query, because the builder sets `preserve_order`. A plain `LIMIT` was the only exception, and that exception is the same path that returns wrong rows. On the same table, `main` with no `LIMIT` already pays for the uneven groups:
| Query (pushdown_filters off) | main, `WITH ORDER` | main, no declared order |
|---|---|---|
| `SELECT sum(a + b) FROM t WHERE b > 0.5` | 233 | 93 |
| `SELECT * FROM t WHERE b > 0.999999` | 32 | 13 |
Work stealing does not explain this gap. With `enable_file_stream_work_stealing = false`, the no-order table still runs in about 91 ms. The real cause is file grouping. Unordered scans are cut into size-balanced byte ranges. Ordered scans keep `split_files`'s count-based chunks (`[f00, f01], [f02, f03], ...`, two 16M-row partitions), and `repartition_preserving_order` returns early once there are `target_partitions` groups.
Cutting the concatenated groups into contiguous, size-balanced ranges keeps every group sorted, because each new group is a contiguous run in file order. I prototyped that on top of this PR (ms, median of 5 interleaved runs):
| Query | pf | main | this PR | this PR + balanced ordered groups |
|---|---|---|---|---|
| `WHERE b > 0.999999 LIMIT 10` | off / on | 12 / 4 | 14 / 6 | 9 / 4 |
| `sum(a + b) WHERE b > 0.5` | off / on | 233 / 310 | 232 / 310 | 93 / 119 |
| `WHERE b > 0.999999` | off / on | 32 / 30 | 32 / 30 | 14 / 12 |
| `ORDER BY a LIMIT 10` (+ filter variant) | off / on | 3-5 | 3-4 | 3-5 |
So it fixes your case and gives a 2.3-2.5x win on full scans over ordered tables, with no work stealing and no order-sensitivity signal between rules. To stay correct, it keeps a re-cut only when `validate_orderings` keeps the same orderings for the new groups. That changes the file groups shown in EXPLAIN for ordered scans, so I opened it separately as draft #26458 (issue #26457) to keep this PR small. Measured against `main` there: 228 -> 78 ms for the full scan, 11 -> 8 ms for your `LIMIT` query, `ORDER BY` queries unchanged.
##########
datafusion/physical-plan/src/limit.rs:
##########
@@ -347,21 +344,16 @@ impl GlobalLimitExec {
input,
skip,
fetch,
- required_ordering,
+ // Deprecated, ignored.
+ required_ordering: _,
Review Comment:
Added `limit_decode_ignores_required_ordering`. It sets a `required_ordering` on the encoded `GlobalLimitExecNode` / `LocalLimitExecNode` and checks that decoding succeeds with skip/fetch kept. The sort node has no `expr`, so a decoder that still reads the field fails. I checked this: restoring the old decode makes the test fail with `PhysicalSortExpr is missing required field 'expr'`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep file order for scans that advertise an output ordering [datafusion]
Posted by "kosiew (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: keep file order for scans that advertise an output ordering [datafusion]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #26434:
URL: https://github.com/apache/datafusion/pull/26434#issuecomment-6225847219
<!-- semver-check-comment -->
Thank you for opening this pull request!
Reviewer note: [cargo-semver-checks](https://github.com/obi1kenobi/cargo-semver-checks) reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch).
<details>
<summary>Details</summary>
```
Cloning apache/main
Building datafusion v55.1.0 (current)
Built [ 39.300s] (current)
Parsing datafusion v55.1.0 (current)
Parsed [ 0.023s] (current)
Building datafusion v55.1.0 (baseline)
Built [ 36.662s] (baseline)
Parsing datafusion v55.1.0 (baseline)
Parsed [ 0.024s] (baseline)
Checking datafusion v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.417s] 229 checks: 229 pass, 31 skip
Summary no semver update required
Finished [ 78.785s] datafusion
Building datafusion-datasource v55.1.0 (current)
Built [ 27.165s] (current)
Parsing datafusion-datasource v55.1.0 (current)
Parsed [ 0.020s] (current)
Building datafusion-datasource v55.1.0 (baseline)
Built [ 26.704s] (baseline)
Parsing datafusion-datasource v55.1.0 (baseline)
Parsed [ 0.022s] (baseline)
Checking datafusion-datasource v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.190s] 229 checks: 228 pass, 1 fail, 0 warn, 31 skip
--- failure trait_method_marked_deprecated: trait method #[deprecated] added ---
Description:
A trait method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/trait_method_marked_deprecated.ron
Failed in:
method with_preserve_order in trait datafusion_datasource::source::DataSource in /home/runner/work/datafusion/datafusion/datafusion/datasource/src/source.rs:128
Summary semver requires new minor version: 0 major and 1 minor checks failed
Finished [ 55.810s] datafusion-datasource
Building datafusion-datasource-parquet v55.1.0 (current)
Built [ 31.525s] (current)
Parsing datafusion-datasource-parquet v55.1.0 (current)
Parsed [ 0.024s] (current)
Building datafusion-datasource-parquet v55.1.0 (baseline)
Built [ 31.020s] (baseline)
Parsing datafusion-datasource-parquet v55.1.0 (baseline)
Parsed [ 0.023s] (baseline)
Checking datafusion-datasource-parquet v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.122s] 229 checks: 229 pass, 31 skip
Summary no semver update required
Finished [ 63.783s] datafusion-datasource-parquet
Building datafusion-physical-optimizer v55.1.0 (current)
Built [ 24.642s] (current)
Parsing datafusion-physical-optimizer v55.1.0 (current)
Parsed [ 0.014s] (current)
Building datafusion-physical-optimizer v55.1.0 (baseline)
Built [ 24.201s] (baseline)
Parsing datafusion-physical-optimizer v55.1.0 (baseline)
Parsed [ 0.014s] (baseline)
Checking datafusion-physical-optimizer v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.078s] 229 checks: 229 pass, 31 skip
Summary no semver update required
Finished [ 50.028s] datafusion-physical-optimizer
Building datafusion-physical-plan v55.1.0 (current)
Built [ 22.867s] (current)
Parsing datafusion-physical-plan v55.1.0 (current)
Parsed [ 0.134s] (current)
Building datafusion-physical-plan v55.1.0 (baseline)
Built [ 22.011s] (baseline)
Parsing datafusion-physical-plan v55.1.0 (baseline)
Parsed [ 0.139s] (baseline)
Checking datafusion-physical-plan v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.499s] 229 checks: 227 pass, 2 fail, 0 warn, 31 skip
--- failure trait_method_marked_deprecated: trait method #[deprecated] added ---
Description:
A trait method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/trait_method_marked_deprecated.ron
Failed in:
method with_preserve_order in trait datafusion_physical_plan::execution_plan::ExecutionPlan in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/execution_plan.rs:102
method with_preserve_order in trait datafusion_physical_plan::ExecutionPlan in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/execution_plan.rs:102
--- failure type_method_marked_deprecated: type method #[deprecated] added ---
Description:
A type method is now #[deprecated]. Downstream crates will get a compiler warning when using this method.
ref: https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute
impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.51.0/src/lints/type_method_marked_deprecated.ron
Failed in:
method datafusion_physical_plan::limit::LocalLimitExec::required_ordering in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/limit.rs:420
method datafusion_physical_plan::limit::LocalLimitExec::set_required_ordering in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/limit.rs:429
method datafusion_physical_plan::limit::GlobalLimitExec::required_ordering in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/limit.rs:120
method datafusion_physical_plan::limit::GlobalLimitExec::set_required_ordering in /home/runner/work/datafusion/datafusion/datafusion/physical-plan/src/limit.rs:129
Summary semver requires new minor version: 0 major and 2 minor checks failed
Finished [ 46.761s] datafusion-physical-plan
Building datafusion-proto v55.1.0 (current)
Built [ 33.423s] (current)
Parsing datafusion-proto v55.1.0 (current)
Parsed [ 0.009s] (current)
Building datafusion-proto v55.1.0 (baseline)
Built [ 33.293s] (baseline)
Parsing datafusion-proto v55.1.0 (baseline)
Parsed [ 0.010s] (baseline)
Checking datafusion-proto v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.087s] 229 checks: 229 pass, 31 skip
Summary no semver update required
Finished [ 68.057s] datafusion-proto
Building datafusion-proto-models v55.1.0 (current)
Built [ 14.990s] (current)
Parsing datafusion-proto-models v55.1.0 (current)
Parsed [ 0.102s] (current)
Building datafusion-proto-models v55.1.0 (baseline)
Built [ 15.001s] (baseline)
Parsing datafusion-proto-models v55.1.0 (baseline)
Parsed [ 0.104s] (baseline)
Checking datafusion-proto-models v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 1.431s] 229 checks: 229 pass, 31 skip
Summary no semver update required
Finished [ 32.787s] datafusion-proto-models
Building datafusion-sqllogictest v55.1.0 (current)
Built [ 60.108s] (current)
Parsing datafusion-sqllogictest v55.1.0 (current)
Parsed [ 0.014s] (current)
Building datafusion-sqllogictest v55.1.0 (baseline)
Built [ 72.859s] (baseline)
Parsing datafusion-sqllogictest v55.1.0 (baseline)
Parsed [ 0.013s] (baseline)
Checking datafusion-sqllogictest v55.1.0 -> v55.1.0 (no change; assume patch)
Checked [ 0.073s] 229 checks: 229 pass, 31 skip
Summary no semver update required
Finished [ 135.551s] datafusion-sqllogictest
```
</details>
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org