[fix](be) Refresh V2 predicates and bound nested Parquet skips#65498
[fix](be) Refresh V2 predicates and bound nested Parquet skips#65498Gabriel39 wants to merge 8 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScannerV2 cloned its table-level conjuncts only when TableReader was
initialized. Runtime filters arriving after scanner open were visible to the scanner but never
reached subsequent split readers, so Parquet and ORC metadata pruning, native file filtering, and
JNI block filtering could not benefit from late runtime filters. Pass a freshly cloned conjunct
snapshot into every split, replace TableReader's initial snapshot during split preparation, and
prepare JNI conjuncts per split.
### Release note
Enable late runtime filters to participate in FileScannerV2 file-level filtering for subsequent
splits.
### Check List (For Author)
- Test: Unit Test
- `TableReaderTest.PrepareSplitReplacesInitialConjunctSnapshot`
- `FileScannerV2Test.RewriteSlotRefsToGlobalIndexMatrix`
- Behavior changed: Yes. Subsequent FileScannerV2 splits use the latest runtime-filter snapshot for
native and JNI filtering.
- Does this need documentation: No
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
I found one correctness issue in the refreshed split-conjunct path. The change installs a fresh split snapshot, but table-level COUNT splits can still return synthetic rows before those filters are built or evaluated.
Critical checkpoint conclusions:
- Goal/test: the PR mostly addresses late runtime-filter refresh for normal native/JNI split filtering and adds a focused unit test, but the table-level COUNT path remains uncovered.
- Scope: the implementation is narrow and focused.
- Concurrency: no new shared-state race found; late RF updates are still cloned through the existing scanner/local-state path.
- Lifecycle: JNI per-split preparation was reviewed; preserved initial contexts remain effectively idempotent due concrete expression prepare guards and idempotent open.
- Config/compatibility: no new config or serialization/storage compatibility issue found.
- Parallel paths: native/JNI/hybrid forwarding is covered, but COUNT/table_level_row_count is a parallel fast path that bypasses the refreshed predicates.
- Tests: added coverage validates request replacement, but not refreshed conjuncts combined with table_level_row_count.
- Observability/performance/persistence: no additional issue found.
- User focus: no additional user-provided focus points were supplied.
Subagent conclusions:
- tests-session-config-001 was accepted as the inline comment below.
- optimizer-rewrite-001 was dismissed after verifying expression prepare/open idempotence for preserved JNI snapshots.
- Convergence round 1 ended with both live subagents reporting NO_NEW_VALUABLE_FINDINGS for this same ledger/comment set.
Validation note: I attempted ./run-be-ut.sh --run --filter=TableReaderTest.PrepareSplitReplacesInitialConjunctSnapshot, but this checkout failed during BE UT configure because thirdparty/installed/bin/protoc is missing, so the test binary did not run.
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: Complex Parquet column readers materialized every discarded row in a selection gap into one scratch column, so memory and decoding work grew with the entire gap. In addition, a refreshed split predicate could be bypassed by the table-level COUNT shortcut. Materialize skipped nested rows in bounded batches with per-batch scratch columns, and only use metadata row counts when the split has no active conjuncts.
### Release note
Bound memory usage while skipping nested Parquet columns and preserve late runtime-filter semantics for table-level COUNT.
### Check List (For Author)
- Test: Unit Test
- `ParquetColumnReaderControlTest.*` (16 tests)
- `TableReaderTest.PrepareSplitReplacesInitialConjunctSnapshot`
- `TableReaderTest.RefreshedConjunctDisablesTableLevelCount`
- Behavior changed: Yes. Nested Parquet skip work is processed in bounded batches, and table-level COUNT no longer bypasses active split predicates.
- Does this need documentation: No
Gabriel39
changed the title
|
/review |
|
run buildall |
1 similar comment
|
run buildall |
|
/review |
There was a problem hiding this comment.
Requesting changes for one correctness issue in the table-level COUNT shortcut.
Summary: the per-split conjunct refresh now covers newly prepared splits, native reader requests, JNI filtering, and hybrid delegation, and the parquet nested skip batching looks semantically safe in static review. However, an already-prepared large table-level COUNT split can keep emitting synthetic metadata rows after a runtime filter arrives between scheduler reads, so the accepted inline comment should be addressed.
Checkpoint conclusions: goal is mostly met but incomplete for the active-split COUNT/RF lifecycle; scope is focused; no new config, persistence, protocol, or data-write behavior is involved; concurrency risk is a scheduler/lifecycle timing gap rather than a lock issue; parallel native/JNI/hybrid paths were checked; parquet nested skip preserves the existing load/build cursor path and has focused unit coverage; validation was static only per the review prompt, so no builds or tests were run.
User focus: no additional user-provided review focus was supplied.
There was a problem hiding this comment.
Static review completed for PR #65498.
I did not find a new, non-duplicate issue to request changes on. The existing inline thread on TableReader::prepare_split() already covered the table-level COUNT bypass for refreshed split conjuncts; the current patch addresses that with the _conjuncts.empty() guard and the new RefreshedConjunctDisablesTableLevelCount unit test, so I am not duplicating it.
Critical checkpoint conclusions:
- Goal and tests: the PR refreshes V2 table-reader predicates per split and bounds nested Parquet skip scratch materialization; the added BE unit tests cover split conjunct replacement, table-level count disabling with refreshed conjuncts, and bounded nested skip batches.
- Scope and parallel paths: the change is focused. Native TableReader, JNI readers, and Paimon/Hudi hybrid delegation were checked; the fresh
SplitReadOptions::conjunctssnapshot reaches each split path. - Concurrency and lifecycle: no new shared-state concurrency was introduced. JNI scanner open/close and VExpr prepare/open behavior were checked for repeated splits.
- Correctness and pushdown: table-level COUNT and aggregate pushdown were checked against refreshed row predicates and runtime filters. No additional bypass beyond the existing reviewed thread was found.
- Nested Parquet skip: LIST/MAP/STRUCT skip now reuses the old load/build protocol in bounded chunks; cursor advancement, null/empty parents, repeated children, and map key/value alignment were reviewed.
- Compatibility, persistence, configs, writes, observability: no new persistent format, FE/BE protocol, config, write-transaction, or observability requirement was introduced by this PR.
- User focus: no additional user-provided review focus was present.
Validation was static only in this review runner: .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc are absent, so I did not run BE unit tests or builds. The bundle patch reverse-applies cleanly to the checked-out PR head (7f06a4677d9e12cabc91f164e16814d1123fb2bc) with whitespace checking.
### What problem does this PR solve? Issue Number: None Related PR: apache#65498 Problem Summary: A table-level metadata COUNT split can emit several synthetic batches across scheduler turns. If a runtime filter was still pending when the shortcut started, it could arrive after an unfiltered batch had already escaped, and the active split could not recover the corresponding real file rows. Pass the scanner runtime-filter completion state into split preparation and allow metadata COUNT only after every assigned runtime filter has arrived. ### Release note Table-level metadata COUNT now falls back to reading file rows while runtime filters are pending. ### Check List (For Author) - Test: Unit Test - TableReaderTest.PrepareSplitReplacesInitialConjunctSnapshot - TableReaderTest.RefreshedConjunctDisablesTableLevelCount - TableReaderTest.PendingRuntimeFilterDisablesTableLevelCount - Remote BE build - Behavior changed: Yes (metadata COUNT waits for all scanner runtime filters before using synthetic rows) - Does this need documentation: No
|
/review |
|
run buildall |
There was a problem hiding this comment.
Review completed with one remaining correctness issue.
Critical checkpoint conclusions:
- Goal/test coverage: the PR addresses late-runtime-filter handling for V2 table-reader shortcuts and bounded nested Parquet skip memory. The new tests cover fresh conjunct snapshots, pending RFs for table-level metadata COUNT, and bounded nested skip batching, but there is no coverage for pending RFs with the normal aggregate-pushdown path.
- Scope/focus: the changes are focused and mostly align with existing TableReader, FileScannerV2, JNI reader, and Parquet nested-reader patterns.
- Concurrency/lifecycle: the relevant lifecycle is scanner scheduler turns and late RF refresh. The table-level metadata COUNT path is now guarded, but the separate aggregate-pushdown path can still emit synthetic rows before a later RF refresh.
- Compatibility/config/persistence: no new config, persistence, transaction, or FE-BE protocol incompatibility was found.
- Parallel paths: JNI does not use the base aggregate-pushdown branch; native Parquet/ORC/text paths do, and the pending-RF state is not carried into that branch.
- Parquet nested skip: the chunked skip helper reuses the old load/build/verify path per parent-row chunk and no LIST/MAP/STRUCT correctness issue was found.
No additional user-provided focus was supplied. This was a static review only; no builds or tests were run in the review-only checkout.
### What problem does this PR solve? Issue Number: close apache#65498 Related PR: apache#65498 Problem Summary: Nested Parquet skips rebuilt parent boundaries into scratch Columns and decoded leaf payloads that were immediately discarded. In addition, pending runtime filters disabled table-level metadata COUNT but could still allow normal COUNT or MIN/MAX aggregate pushdown to emit irreversible synthetic rows before the filter arrived. Add a levels-only nested consume protocol for scalar, list, map, and struct readers, preserving nullability and stream-alignment validation without payload materialization. Gate every aggregate pushdown path on the split runtime-filter snapshot. ### Release note Improve nested Parquet skip efficiency and preserve runtime-filter correctness for aggregate pushdown. ### Check List (For Author) - Test: Regression test / Unit Test - ParquetColumnReaderTest, ParquetColumnReaderBaseTest, ParquetColumnReaderControlTest, and targeted TableReader tests - Remote ./build.sh --be - Behavior changed: Yes (nested skip avoids decoding discarded payloads; aggregate pushdown waits for pending runtime filters) - Does this need documentation: No
|
run buildall |
|
/review |
|
Codex automated review failed and did not complete. Error: You've hit your usage limit. Visit https://chatgpt.com/codex/settings/usage to purchase more credits or try again at 5:19 PM. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
TPC-H: Total hot run time: 29179 ms |
TPC-DS: Total hot run time: 179918 ms |
ClickBench: Total hot run time: 25.04 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: close apache#65498 Related PR: apache#65498 Problem Summary: The Parquet levels-only nested path called Arrow RecordReader::Reset() without first releasing ByteArray and FLBA builder chunks. Arrow requires GetBuilderChunks() to reset those builders, so a nested skip followed by a normal read mixed values from both batches and failed with values greater than the current definition/repetition levels. Release and immediately discard binary chunks after copying levels, without copying payloads into Doris Columns. Also derive the scalar consume slot threshold from schema metadata because levels-only batches intentionally omit value-slot metadata. ### Release note Fix nested Parquet queries that skip filtered rows before reading plain-encoded string values. ### Check List (For Author) - Test: Unit Test - 95 targeted Parquet reader, levels-only COUNT, and TableReader tests - Plain-encoded nested MAP skip-then-read regression test - Remote ./build.sh --be - Behavior changed: Yes (levels-only nested reads correctly reset Arrow binary builders) - Does this need documentation: No
|
run buildall |
|
/review |
### What problem does this PR solve? Issue Number: None Related PR: apache#65498 Problem Summary: Split-level conjunct refresh can expose late runtime filters to TableReader. VARBINARY predicates are intentionally unsupported for external-reader pushdown, but ColumnMapper treated direct VARBINARY mappings as COPY_DIRECTLY and localized late filters into file readers. This caused Iceberg BINARY partition runtime-filter queries to drop matching rows. Mark VARBINARY mappings FINALIZE_ONLY so Scanner evaluates them after materialization while the refreshed snapshot still gates unsafe aggregate shortcuts. ### Release note Fix incorrect results for runtime filters on external VARBINARY columns, including Iceberg BINARY partition columns. ### Check List (For Author) - Test: Unit Test - Remote ASAN BE unit tests: ColumnMapperLocalizeFiltersTest.* and TableReaderTest.PrepareSplitReplacesInitialConjunctSnapshot (6 passed) - Iceberg regression attempted but skipped because remote config disables Iceberg external tests - Behavior changed: Yes (VARBINARY runtime filters stay on scanner-level evaluation path) - Does this need documentation: No
|
run buildall |
|
run buildall |
|
/review |
There was a problem hiding this comment.
Static review found two correctness issues, so I am requesting changes.
Critical checkpoints: runtime-filter aggregate shortcuts were checked and the remaining known cases are already covered by existing review threads; split conjunct and JNI lifecycle were checked with no distinct issue; Parquet nested consume found M-1; live-head VARBINARY filter localization found M-2; test coverage gaps C-1/C-2 were dismissed as not independently substantiated, with regressions requested for M-1 and M-2. No additional user-provided review focus was present.
Validation: static review only. I did not run builds or tests because the review prompt forbids builds for this review environment.
Protocol note: this automated review is incomplete under the prompt's convergence protocol because M-2 was discovered in round 3, the maximum allowed round, so no fourth convergence round was run. All currently known candidates were still accepted or dismissed before submission.
| } | ||
| scalar_key_reader->set_nested_build_level_cursor(level_idx); | ||
| } else { | ||
| RETURN_IF_ERROR(_key_reader->consume_nested_column(static_cast<int64_t>(total_entries), |
There was a problem hiding this comment.
consume_nested_column() is documented to preserve the same null/alignment validation as build_nested_column(), but this non-scalar key branch only checks that the complex key reader consumed the expected count. MAP schemas intentionally accept optional key fields for compatibility, and the materialized path still rejects actual null keys by checking the appended key column. With a nullable STRUCT/LIST MAP key, skipping a row now takes this branch; the complex key reader accepts the null key as a valid nullable value and returns the expected count, so malformed data is silently accepted whenever that row is skipped. Please make the consume-only MAP key path validate that every key entry is non-null regardless of key reader kind, and add a skip regression for an optional complex key containing a null key.
| // physical binary representations are not uniformly supported by reader-side expression and | ||
| // metadata filtering, so localizing a late runtime filter here can incorrectly reject rows. | ||
| if (remove_nullable(mapping.table_type)->get_primitive_type() == TYPE_VARBINARY) { | ||
| return FilterConversionType::FINALIZE_ONLY; |
There was a problem hiding this comment.
This keeps projected VARBINARY filters above the reader, but it breaks predicate-only VARBINARY filters. Hidden filter mappings are created after the visible projected_columns loop, and FINALIZE_ONLY makes filter_conversion_has_local_source() false, so localize_filters() skips the hidden VARBINARY mapping instead of adding it as a predicate column. It is also never added as a non-predicate column, and TableReader::finalize_chunk() only materializes visible mappings(), not hidden mappings. A query that filters on a non-projected VARBINARY column therefore reaches the scanner-side fallback without the VARBINARY slot in the table block. Please make predicate-only VARBINARY filters materialize a column for final evaluation while still avoiding reader-local predicate pushdown, and add a regression where the output projects a different column and the VARBINARY column appears only in the filter.
|
Codex automated review failed and did not complete. Error: Codex completed, but no new pull request review was submitted for the current head SHA. Please inspect the workflow logs and rerun the review after the underlying issue is resolved. |
### What problem does this PR solve? Issue Number: None Related PR: apache#65498 Problem Summary: The VARBINARY external-reader filter fix and its unit test were committed to this PR by mistake. Restore the two affected files to their state before the misplaced commits so this PR contains only its intended changes. ### Release note None ### Check List (For Author) - Test: No need to test (the commit only restores the exact pre-change file contents) - Behavior changed: No - Does this need documentation: No
There was a problem hiding this comment.
Requesting changes for one non-duplicate VARBINARY localization issue found on reviewed commit 5b3c674.
Process note: before submission, the live PR head moved to 77a59cb. The review prompt names 5b3c674 and forbids refreshing the authoritative diff through GitHub, so this inline comment is anchored to the reviewed bundle commit. Live API checks also showed two comments raised after the bundle snapshot: comment 3567612022 already covers the hidden predicate-only VARBINARY materialization issue, and comment 3567612017 already covers a Parquet MAP consume-only key-validation issue, so I did not duplicate either one.
Critical checkpoint conclusions: the split-level runtime-filter snapshot, TableReader metadata shortcut gates, JNI split conjunct lifecycle, and nested Parquet skip batching were reviewed against the bundle diff and existing/live comments. The remaining non-duplicate problem is that the new VARBINARY guard is top-level-only, while nested STRUCT and same-shape ARRAY/MAP predicates can still be localized into external reader conjuncts. The change is otherwise focused and does not add persistent format, config, transaction, FE-BE protocol, or observability requirements. Test coverage now includes visible top-level VARBINARY, but not nested STRUCT<... VARBINARY ...> or same-shape ARRAY/MAP predicates with VARBINARY leaves. No additional user-provided focus was supplied.
Validation: static review only; no builds or tests were run in this review-only checkout.
| return FilterConversionType::FINALIZE_ONLY; | ||
| } | ||
| return mapping.is_trivial ? FilterConversionType::COPY_DIRECTLY | ||
| : FilterConversionType::CAST_FILTER; |
There was a problem hiding this comment.
Please extend this VARBINARY guard to recursive complex dependencies, not just the top-level primitive mapping. For s STRUCT<bin VARBINARY>, the bin child mapping gets FINALIZE_ONLY, but the root struct can still be COPY_DIRECTLY/READER_EXPRESSION; localize_filters() decides from the root global index and rewrite_struct_element_path_to_file_expr() can still push element_at(s, 'bin') = ... into file_request->conjuncts. The same root-type-only check also leaves same-shape ARRAY<VARBINARY> or MAP key/value VARBINARY roots local, so generic expressions such as array_contains(arr, ...) or array_contains(map_keys(m), ...) can be localized through rewrite_table_expr_to_file_expr(). In all of these cases the external reader can reject rows using VARBINARY physical binary semantics before scanner-side final evaluation can correct them. Please make filter localization detect terminal/nested VARBINARY dependencies before adding reader-local predicate columns/conjuncts, with regressions for a VARBINARY struct child and same-shape ARRAY/MAP VARBINARY predicates. The hidden predicate-only VARBINARY materialization case is already covered by a live thread in this PR, so I am not duplicating it here.
### What problem does this PR solve? Issue Number: None Related PR: apache#65498 Problem Summary: A table-level metadata COUNT split can emit several synthetic batches across scheduler turns. If a runtime filter was still pending when the shortcut started, it could arrive after an unfiltered batch had already escaped, and the active split could not recover the corresponding real file rows. Pass the scanner runtime-filter completion state into split preparation and allow metadata COUNT only after every assigned runtime filter has arrived. ### Release note Table-level metadata COUNT now falls back to reading file rows while runtime filters are pending. ### Check List (For Author) - Test: Unit Test - TableReaderTest.PrepareSplitReplacesInitialConjunctSnapshot - TableReaderTest.RefreshedConjunctDisablesTableLevelCount - TableReaderTest.PendingRuntimeFilterDisablesTableLevelCount - Remote BE build - Behavior changed: Yes (metadata COUNT waits for all scanner runtime filters before using synthetic rows) - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#65498 Related PR: apache#65498 Problem Summary: Nested Parquet skips rebuilt parent boundaries into scratch Columns and decoded leaf payloads that were immediately discarded. In addition, pending runtime filters disabled table-level metadata COUNT but could still allow normal COUNT or MIN/MAX aggregate pushdown to emit irreversible synthetic rows before the filter arrived. Add a levels-only nested consume protocol for scalar, list, map, and struct readers, preserving nullability and stream-alignment validation without payload materialization. Gate every aggregate pushdown path on the split runtime-filter snapshot. ### Release note Improve nested Parquet skip efficiency and preserve runtime-filter correctness for aggregate pushdown. ### Check List (For Author) - Test: Regression test / Unit Test - ParquetColumnReaderTest, ParquetColumnReaderBaseTest, ParquetColumnReaderControlTest, and targeted TableReader tests - Remote ./build.sh --be - Behavior changed: Yes (nested skip avoids decoding discarded payloads; aggregate pushdown waits for pending runtime filters) - Does this need documentation: No
### What problem does this PR solve? Issue Number: close apache#65498 Related PR: apache#65498 Problem Summary: The Parquet levels-only nested path called Arrow RecordReader::Reset() without first releasing ByteArray and FLBA builder chunks. Arrow requires GetBuilderChunks() to reset those builders, so a nested skip followed by a normal read mixed values from both batches and failed with values greater than the current definition/repetition levels. Release and immediately discard binary chunks after copying levels, without copying payloads into Doris Columns. Also derive the scalar consume slot threshold from schema metadata because levels-only batches intentionally omit value-slot metadata. ### Release note Fix nested Parquet queries that skip filtered rows before reading plain-encoded string values. ### Check List (For Author) - Test: Unit Test - 95 targeted Parquet reader, levels-only COUNT, and TableReader tests - Plain-encoded nested MAP skip-then-read regression test - Remote ./build.sh --be - Behavior changed: Yes (levels-only nested reads correctly reset Arrow binary builders) - Does this need documentation: No
TPC-H: Total hot run time: 29645 ms |
TPC-DS: Total hot run time: 180919 ms |
ClickBench: Total hot run time: 25.23 s |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScannerV2 cloned table-level conjuncts only during TableReader initialization. Runtime filters arriving after scanner open were visible to the scanner but not to later split readers, so native and JNI file-level filtering could not use them. The table-level COUNT shortcut could also return synthetic rows for a split without evaluating a refreshed predicate.
Separately, LIST, MAP, and STRUCT Parquet readers implemented
skip()by materializing the complete discarded range into one scratch column. Large selection gaps therefore caused memory and decoding work to grow with the entire gap.This change passes a fresh conjunct snapshot into every split, prepares JNI conjuncts per split, and disables metadata-only table counts whenever a split has active conjuncts. Nested Parquet skips now reuse a shared bounded-batch path that materializes at most 4096 parent rows per scratch column and releases each scratch column before continuing.
Release note
Refresh FileScannerV2 predicates for each split, preserve filtered COUNT semantics, and bound memory usage while skipping nested Parquet columns.
Check List (For Author)
ParquetColumnReaderControlTest.*(16 tests passed)TableReaderTest.PrepareSplitReplacesInitialConjunctSnapshotTableReaderTest.RefreshedConjunctDisablesTableLevelCount