alamb commented on code in PR #24359:
URL: https://github.com/apache/datafusion/pull/24359#discussion_r3784058623
##########
datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt:
##########
@@ -194,3 +194,78 @@ RESET datafusion.optimizer.enable_dynamic_filter_pushdown;
statement ok
RESET datafusion.optimizer.enable_topk_dynamic_filter_pushdown;
+
+# Regression test for #24355: the runtime row-group pruner rebuilds the decoder
Review Comment:
Can we please update this description to focus on what properties the test
has rather than what was wrong with the current implementation (which will
become immediately out of date once this PR is merged)
For example, I think the key properties of this file is that the the dynamic
predicate ends up pruning a row group during the application of multiple
predicates
##########
datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt:
##########
@@ -194,3 +194,78 @@ RESET datafusion.optimizer.enable_dynamic_filter_pushdown;
statement ok
RESET datafusion.optimizer.enable_topk_dynamic_filter_pushdown;
+
+# Regression test for #24355: the runtime row-group pruner rebuilds the decoder
+# via `into_builder().with_row_groups(...)`, which drops row groups without
+# slicing the carried page-index `RowSelection` to match — a dropped RG's
+# selectors are then applied to the next surviving RG. Layout (RG size 100):
+# RG 0: b=1000..1099, a=100..199 (a>=50 keeps all)
+# RG 1: b=2000..2099, a=0..99 (a>=50 keeps rows 50..99 — page-index
prunes
+# the first 5 pages, leaving `skip 50,
select 50`)
+# RG 2: b=3000..3099, a=100..199 (keeps all)
+# RG 3: b=0..99, a=100..199 (keeps all)
+# `ORDER BY b ASC LIMIT 5` tightens the TopK threshold; the runtime pruner
drops
+# RG 1 and RG 2, rebuilds with row_groups=[3], but the unsliced `skip 50,
+# select 50` is applied to RG 3 — dropping b=0..49, which is the correct
answer.
+# Without the fix (#24355) this returns 50..54; with it the pruner is disabled
+# while a row selection is live, so the answer is correct.
+# `data_page_row_count_limit`/`write_batch_size` force multiple pages per RG so
+# page-index pruning can produce an intra-RG selection.
+statement ok
+set datafusion.execution.target_partitions = 1;
+
+statement ok
+set datafusion.execution.parquet.pushdown_filters = true;
+
+statement ok
+CREATE TABLE rgsel_src AS
+SELECT
+ CAST(CASE WHEN i / 100 = 1 THEN i % 100 ELSE 100 + (i % 100) END AS BIGINT)
AS a,
+ CAST(CASE
+ WHEN i < 100 THEN 1000 + i
+ WHEN i < 200 THEN 2000 + (i - 100)
+ WHEN i < 300 THEN 3000 + (i - 200)
+ ELSE (i - 300)
+ END AS BIGINT) AS b
+FROM generate_series(0, 399) AS t(i);
+
+statement ok
+COPY (SELECT * FROM rgsel_src)
+TO 'test_files/scratch/dynamic_row_group_pruning/rgsel.parquet'
+STORED AS PARQUET
+OPTIONS (
+ 'format.max_row_group_size' '100',
+ 'format.data_page_row_count_limit' '10',
+ 'format.write_batch_size' '10'
+);
+
+statement ok
+drop table rgsel_src;
+
+statement ok
+CREATE EXTERNAL TABLE rgsel (a BIGINT NOT NULL, b BIGINT NOT NULL)
+STORED AS PARQUET
+LOCATION 'test_files/scratch/dynamic_row_group_pruning/rgsel.parquet';
+
+# The correct top-5 by `b` among rows with `a >= 50` is b = 0..4 (they live in
Review Comment:
Can you also please update the test so it runs the same query without filter
pushdown so it is clear the answers are the same?
##########
datafusion/core/tests/parquet/dynamic_row_group_pruning.rs:
##########
@@ -297,9 +297,12 @@ fn build_five_thousand_row_rgs(schema: &Arc<Schema>) ->
Vec<RecordBatch> {
.collect()
}
-/// Co-existence test for **page-index `RowSelection`** + dynamic RG
-/// pruning. Tests that the `into_builder` rebuild preserves the
-/// `RowSelection` derived from page-index pruning across RG drops.
+/// Regression test for #24355: when a page-index `RowSelection` is live,
+/// the runtime dynamic row-group pruner is intentionally **not built**, so
+/// its `into_builder` rebuild can never drop a row group without slicing the
+/// carried selection (which would silently return wrong rows). Correctness is
+/// bought at the cost of the dynamic-pruning optimization for this scan; the
+/// proper fix that keeps both is tracked upstream in arrow-rs #10624 / #24358.
Review Comment:
can you make these actual github links (so it is clear to which repo they
belong and is easier to follow the links)
##########
datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt:
##########
@@ -194,3 +194,78 @@ RESET datafusion.optimizer.enable_dynamic_filter_pushdown;
statement ok
RESET datafusion.optimizer.enable_topk_dynamic_filter_pushdown;
+
+# Regression test for #24355: the runtime row-group pruner rebuilds the decoder
+# via `into_builder().with_row_groups(...)`, which drops row groups without
+# slicing the carried page-index `RowSelection` to match — a dropped RG's
+# selectors are then applied to the next surviving RG. Layout (RG size 100):
+# RG 0: b=1000..1099, a=100..199 (a>=50 keeps all)
+# RG 1: b=2000..2099, a=0..99 (a>=50 keeps rows 50..99 — page-index
prunes
+# the first 5 pages, leaving `skip 50,
select 50`)
+# RG 2: b=3000..3099, a=100..199 (keeps all)
+# RG 3: b=0..99, a=100..199 (keeps all)
+# `ORDER BY b ASC LIMIT 5` tightens the TopK threshold; the runtime pruner
drops
+# RG 1 and RG 2, rebuilds with row_groups=[3], but the unsliced `skip 50,
+# select 50` is applied to RG 3 — dropping b=0..49, which is the correct
answer.
+# Without the fix (#24355) this returns 50..54; with it the pruner is disabled
+# while a row selection is live, so the answer is correct.
+# `data_page_row_count_limit`/`write_batch_size` force multiple pages per RG so
+# page-index pruning can produce an intra-RG selection.
+statement ok
+set datafusion.execution.target_partitions = 1;
+
+statement ok
+set datafusion.execution.parquet.pushdown_filters = true;
+
+statement ok
+CREATE TABLE rgsel_src AS
+SELECT
+ CAST(CASE WHEN i / 100 = 1 THEN i % 100 ELSE 100 + (i % 100) END AS BIGINT)
AS a,
+ CAST(CASE
+ WHEN i < 100 THEN 1000 + i
+ WHEN i < 200 THEN 2000 + (i - 100)
+ WHEN i < 300 THEN 3000 + (i - 200)
+ ELSE (i - 300)
+ END AS BIGINT) AS b
+FROM generate_series(0, 399) AS t(i);
+
+statement ok
+COPY (SELECT * FROM rgsel_src)
+TO 'test_files/scratch/dynamic_row_group_pruning/rgsel.parquet'
+STORED AS PARQUET
+OPTIONS (
+ 'format.max_row_group_size' '100',
+ 'format.data_page_row_count_limit' '10',
+ 'format.write_batch_size' '10'
+);
+
+statement ok
+drop table rgsel_src;
+
+statement ok
+CREATE EXTERNAL TABLE rgsel (a BIGINT NOT NULL, b BIGINT NOT NULL)
+STORED AS PARQUET
+LOCATION 'test_files/scratch/dynamic_row_group_pruning/rgsel.parquet';
+
+# The correct top-5 by `b` among rows with `a >= 50` is b = 0..4 (they live in
+# RG 3, all of whose rows satisfy `a >= 50`). The bug returns 50..54.
+query I
+SELECT b FROM rgsel WHERE a >= 50 ORDER BY b ASC LIMIT 5;
Review Comment:
I reverted the code change in this PR and ran `cargo test --profile=ci
--test sqllogictests -- dynamic_row_group_pruning.slt`
and it fails like this
```sql
. query result mismatch:
[SQL] SELECT b FROM rgsel WHERE a >= 50 ORDER BY b ASC LIMIT 5;
[Diff] (-expected|+actual)
- 0
- 1
- 2
- 3
- 4
+ 50
+ 51
+ 52
+ 53
+ 54
at
/private/tmp/df-24359-ablation/datafusion/sqllogictest/test_files/dynamic_row_group_pruning.slt:252
```
(as expected)
In case anyone else is interested in what is in the file
<details>
```sql
> select * from
'./datafusion/sqllogictest/test_files/scratch/dynamic_row_group_pruning/rgsel.parquet';
+-----+------+
| a | b |
+-----+------+
| 100 | 1000 |
| 101 | 1001 |
| 102 | 1002 |
| 103 | 1003 |
| 104 | 1004 |
| 105 | 1005 |
| 106 | 1006 |
| 107 | 1007 |
| 108 | 1008 |
| 109 | 1009 |
| 110 | 1010 |
| 111 | 1011 |
| 112 | 1012 |
| 113 | 1013 |
| 114 | 1014 |
| 115 | 1015 |
| 116 | 1016 |
| 117 | 1017 |
| 118 | 1018 |
| 119 | 1019 |
| 120 | 1020 |
| 121 | 1021 |
| 122 | 1022 |
| 123 | 1023 |
| 124 | 1024 |
| 125 | 1025 |
| 126 | 1026 |
| 127 | 1027 |
| 128 | 1028 |
| 129 | 1029 |
| 130 | 1030 |
| 131 | 1031 |
| 132 | 1032 |
| 133 | 1033 |
| 134 | 1034 |
| 135 | 1035 |
| 136 | 1036 |
| 137 | 1037 |
| 138 | 1038 |
| 139 | 1039 |
| . |
| . |
| . |
+-----+------+
```
And the whole results
```sql
> SELECT a, b FROM
'./datafusion/sqllogictest/test_files/scratch/dynamic_row_group_pruning/rgsel.parquet'
WHERE a >= 50 ORDER BY b ASC LIMIT 5;
+-----+---+
| a | b |
+-----+---+
| 100 | 0 |
| 101 | 1 |
| 102 | 2 |
| 103 | 3 |
| 104 | 4 |
+-----+---+
```
</details>
##########
datafusion/datasource-parquet/src/opener/mod.rs:
##########
@@ -1464,6 +1464,15 @@ impl RowGroupsPrunedParquetOpen {
};
let prepared_access_plan = prepare_access_plan(access_plan)?;
+ // #24355: a row selection (from page-index pruning, or an
externally
+ // supplied `ParquetRowSelection`) is carried by the decoder as one
+ // flat selection over the concatenation of the remaining row
groups.
+ // The runtime pruner's `into_builder().with_row_groups(...)`
rebuild
Review Comment:
another good place to leave a link to the proper ticket fix
##########
datafusion/core/tests/parquet/dynamic_row_group_pruning.rs:
##########
@@ -309,17 +312,14 @@ fn build_five_thousand_row_rgs(schema: &Arc<Schema>) ->
Vec<RecordBatch> {
/// first 5 pages (values 0..500) are pruned, the last 5 (500..1000)
/// are scanned. RGs 1..4 keep all their pages (every page has
/// `max >= 500`). The decoder receives a `RowSelection` that masks
-/// out those first 5 pages of RG 0.
-/// - `ORDER BY v DESC LIMIT 5` fills the TopK heap from RG 4
-/// (`max=4999`); the tightened threshold (≥ 4995) then proves RGs
-/// 0..3 unreachable and the runtime pruner drops them in one
-/// `into_builder` rebuild.
-///
-/// If `into_builder` did **not** preserve the row selection (or
-/// truncated / shifted it incorrectly), either the result rows would
-/// drift or the count of pruned pages would drop to zero.
+/// out those first 5 pages of RG 0 — its presence is what suppresses
+/// the runtime pruner.
+/// - `ORDER BY v DESC LIMIT 5` would let the tightened TopK threshold
+/// (≥ 4995) prune RGs 0..3, but because a row selection is present the
Review Comment:
maybe here would be a better place to add the tracking ticket for the change
in behavior so it is clear what is expected to change when this feature is
implemented
--
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]
