[fix](be) Harden external file filtering, statistics, and nested reads#65500
Open
Gabriel39 wants to merge 22 commits into
Open
[fix](be) Harden external file filtering, statistics, and nested reads#65500Gabriel39 wants to merge 22 commits into
Gabriel39 wants to merge 22 commits into
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FileScannerV2 treated a missing optional Parquet null_count as proof that a row group contained no NULL values. When min/max statistics were present, IS NULL predicates could therefore prune a row group that actually contained NULL rows. Treat unknown null counts conservatively as possibly containing NULL while retaining available min/max statistics, and cover the metadata combination with a unit test.
### Release note
Fix incorrect IS NULL results for Parquet files whose writers omit null_count statistics.
### Check List (For Author)
- Test: Unit Test
- ParquetStatistics* BE unit tests
- Behavior changed: Yes, row groups with unknown Parquet null counts are no longer discarded by IS NULL pruning
- Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#64456 Problem Summary: FileScannerV2 converts UTC timestamp statistics into session-local DATETIMEV2 values before ZoneMap evaluation. Across a daylight-saving time rollback, that conversion is not monotonic, so converted metadata endpoints may exclude local values that occur inside the UTC interval. Parquet row-group and page-index pruning, and ORC statistics pruning, could therefore skip matching data. Detect backward timezone transitions within UTC statistics ranges and conservatively disable min/max pruning for those ranges while retaining statistics for monotonic ranges and TIMESTAMPTZ mappings. ### Release note Fix incorrect Parquet and ORC file scan pruning for UTC timestamps across daylight-saving time rollbacks. ### Check List (For Author) - Test: Unit Test - Remote ASAN BE UT: TimestampStatisticsTest.*, ParquetStatisticsTransformTest.*, and NewOrcReaderTest.AggregatePushdownTimestamp* (7 passed) - Behavior changed: Yes. Non-monotonic local timestamp statistics no longer prune rows or pages. - Does this need documentation: No
Disable unsafe ORC timestamp SARG pushdown in civil years with backward timezone transitions, while preserving Parquet min/max pruning for TIMESTAMPTZ values that retain UTC ordering. Add ORC, Parquet, and timezone transition regression coverage.
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: Parquet row-group pruning cached Bloom filters only by leaf column id. After the first row group loaded a column Bloom filter, later row groups reused that filter and could be incorrectly pruned when a matching value existed only outside the first row group. Scope cache entries by both row group and leaf column so absence checks always use the current row group's Bloom filter. Add a parquet-mr fixture with two row groups and real Bloom data plus an enabled/disabled differential unit test.
### Release note
Fix incorrect Parquet Bloom filter pruning that could silently omit rows from later row groups.
### Check List (For Author)
- Test: Unit Test
- ParquetStatisticsPruningTest.BloomFilterCacheIsScopedToRowGroupAndColumn
- Behavior changed: Yes, Parquet Bloom pruning now uses each row group's own filter
- Does this need documentation: No
Issue Number: N/A
Related PR: N/A
Problem Summary: Parquet FLOAT and DOUBLE min/max statistics containing NaN were treated as valid by FileScannerV2. Doris comparisons can then interpret NaN as equal to predicate bounds, causing range and inequality predicates to incorrectly prune row groups or pages that contain matching finite values. Reject footer and ColumnIndex min/max statistics when either floating-point bound is NaN, while preserving null-count information and finite statistics.
Fix missing rows when Parquet floating-point min/max statistics contain NaN.
- Test: Unit Test
- ParquetStatisticsTransformTest targeted NaN footer and ColumnIndex tests
- Behavior changed: Yes, NaN floating-point min/max statistics are ignored for pruning.
- Does this need documentation: No
Issue Number: N/A Related PR: apache#65493 Problem Summary: A NaN min/max in one Parquet ColumnIndex page was reported as a transform failure, which disabled page-index pruning for the entire slot. Treat NaN bounds as unavailable only for that page so it is kept conservatively while later pages with finite bounds remain eligible for pruning. Preserve Parquet page-index pruning for finite pages when another page has NaN statistics. - Test: Unit Test - ParquetStatisticsTransformTest.* (4 tests passed) - Behavior changed: Yes. NaN page bounds no longer disable pruning for other pages in the slot. - Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65493 Problem Summary: When NaN invalidated Parquet min/max bounds, the statistics adapter returned no zonemap and discarded otherwise valid null-count metadata. Build a pass-all zonemap instead so range predicates remain conservative while IS NULL and IS NOT NULL can still prune from has_null and has_not_null for both footer and page-index statistics. ### Release note Preserve Parquet null-predicate pruning when floating-point min/max bounds contain NaN. ### Check List (For Author) - Test: Unit Test - ParquetStatisticsTransformTest.* (5 tests passed) - Behavior changed: Yes. Invalid range bounds no longer disable valid null-count pruning. - Does this need documentation: No
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
This was referenced Jul 12, 2026
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Automated review completed for PR 65500 at head 6f4be2243b4fa9c3fe509b46daf961d7652fc932.
I did not find a substantiated correctness issue requiring an inline comment.
Critical checkpoint conclusions:
- Goal and tests: the PR hardens Parquet/ORC metadata pruning for missing null counts, DST rollback timestamp ranges, row-group bloom-filter cache scoping, and NaN min/max statistics. The changed code matches that goal in the reviewed paths, and targeted BE tests were added for each failure mode.
- Scope and focus: the changes are focused on BE format_v2 statistics/SARG pruning and matching unit tests.
- Data correctness: unsafe timestamp or NaN bounds fall back to no range pruning or aggregate pushdown failure; null-count-only pruning preserves only trustworthy
IS NULL/IS NOT NULLdecisions; bloom filters are now keyed by row group and leaf column. - Parallel paths: Parquet footer statistics, page-index statistics, aggregate min/max, ORC zone maps, ORC SARG construction, and TIMESTAMPTZ bypass paths were checked.
- Concurrency, lifecycle, config, persistence, and protocol compatibility: no new concurrency, persistent metadata, runtime config, FE/BE protocol, or storage-format compatibility path is introduced by this PR.
- Performance and observability: the timestamp rollback guard intentionally trades some metadata pruning in rollback years for correctness; no new observability requirement was identified.
- Test coverage: the added tests cover the changed behaviors statically. I did not run builds or tests because the review prompt forbids builds in this runner; the exact base commit is also not present locally, so
git diff --checkon the commit range could not run. I checked the authoritative bundled patch for added-line trailing whitespace instead. - User focus: no additional user-provided review focus was supplied.
### What problem does this PR solve? Issue Number: None Related PR: apache#65449 Problem Summary: FileScannerV2 split-level partition pruning classified predicates only by their referenced slots. A predicate could therefore reference only partition columns while also containing a zero-slot non-deterministic or error-preserving expression. Evaluating such a predicate once before reading the split could change its normal row-level semantics and incorrectly prune the whole file. Check the unwrapped predicate's existing safe-to-reorder contract before evaluating it at split level, and add unit coverage proving an unsafe runtime-filter predicate is neither executed nor used to prune a split. ### Release note Prevent FileScannerV2 partition pruning from pre-evaluating non-deterministic or error-preserving predicates. ### Check List (For Author) - Test: Unit Test - `TableReaderTest.PrepareSplit*` (2 tests passed) - `build-support/clang-format.sh` - `build-support/check-format.sh` - `git diff --check` - Behavior changed: Yes. Unsafe predicates remain on the normal row-level evaluation path instead of being evaluated once per split. - Does this need documentation: No
Issue Number: None Related PR: apache#65449 Problem Summary: Split pruning could skip an unsafe conjunct and still pre-execute later predicates. A later predicate could then prune a split before the earlier non-deterministic or error-preserving predicate reached its normal row-level evaluation point, changing conjunct ordering semantics. Preserve only the safe prefix for V2 partition pruning, V2 constant pruning, and the legacy FileScanner runtime-filter partition pruning path. Add unit coverage for all three paths. Split pruning now preserves row-level conjunct order when predicates are unsafe to pre-execute. - Test: Unit Test - ASAN BE unit tests: TableReaderTest.PrepareSplit*, TableReaderTest.ConstantPruningStopsAtUnsafePredicate, FileScannerTest.PartitionPruningStopsAtUnsafePredicate - C++ format check - Behavior changed: Yes. Split pruning stops at the first predicate that is unsafe to pre-execute. - Does this need documentation: No
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: FileScannerV2 localized predicate literals to the file column type whenever conversion succeeded, even when the conversion changed the literal value. For example, localizing a DOUBLE predicate value of 1.5 to an INT file column changed the boundary to 1 and could make file-level filtering silently drop matching rows. Require an exact round trip through the file type before localizing a literal; otherwise, preserve table semantics by casting the file slot. Exact conversions such as BIGINT 1 to INT 1 remain localized.
### Release note
Fix FileScannerV2 predicate localization to avoid missing rows after lossy literal conversion.
### Check List (For Author)
- Test: Unit Test
- ColumnMapperCastTest targeted literal localization tests
- Behavior changed: Yes, lossy literal conversions now fall back to casting the file slot.
- Does this need documentation: No
### What problem does this PR solve? Issue Number: N/A Related PR: apache#65490 Problem Summary: Round-trip equality alone can accept unsafe predicate localization when file values lose information during table materialization or when complex Field equality ignores nested contents. Restrict localization to scalar numeric file-to-table casts that preserve every file value, retain the exact literal round-trip check, and fall back to the table predicate for complex or unsupported casts. ### Release note Prevent file predicate localization from dropping rows for lossy schema-evolution casts. ### Check List (For Author) - Test: Unit Test - ColumnMapperCastTest (19 tests passed) - Behavior changed: Yes. Unsafe literal localization now falls back to evaluating the table-typed predicate after materialization. - Does this need documentation: No
### What problem does this PR solve?
Issue Number: N/A
Related PR: N/A
Problem Summary: FileScannerV2 returned Parquet footer min/max statistics as exact aggregate values. For BYTE_ARRAY and FIXED_LEN_BYTE_ARRAY columns, Parquet permits truncated lower and upper bounds, and Arrow 17 does not expose the exactness flags. MIN/MAX pushdown could therefore return a synthetic bound that does not occur in the data. Reject aggregate pushdown for these physical types so the upper reader falls back to a normal scan, while retaining exact numeric pushdown and safe statistics pruning.
### Release note
Fix incorrect Parquet MIN/MAX aggregate results for truncated binary statistics.
### Check List (For Author)
- Test: Unit Test
- ParquetScanTest targeted binary rejection and numeric success tests
- Behavior changed: Yes, Parquet binary MIN/MAX aggregate pushdown now falls back to scanning.
- Does this need documentation: No
Gabriel39
changed the title
Contributor
Author
|
/review |
Contributor
Author
|
run buildall |
Contributor
There was a problem hiding this comment.
I found two correctness issues that need changes before merge.
Checkpoint summary:
- Goal/test proof: the PR improves several pruning and metadata conservatism paths, but constant split pruning still has a safe-prefix gap and invalid timestamp metadata can still assert instead of falling back.
- Scope/focus: the changes are focused on BE file scanner and format_v2 metadata paths; I did not find unrelated refactoring issues.
- Parallel paths: legacy and V2 partition pruning now stop at unsafe predicates, but the constant-pruning path still iterates the reduced
_table_filterslist and misses slotless unsafe barriers. - Error handling/data quality: most missing, NaN, unsupported, and non-monotonic metadata cases fall back conservatively; inverted timestamp min/max is the remaining non-conservative case.
- Tests/validation: tests were not run because the review prompt forbids builds/tests in this runner.
.worktree_initializedandthirdparty/installedare missing, andgit diff --checkcould not run because the base object is absent in this shallow/grafted checkout. - User focus: no additional user-provided focus points were present.
- Convergence: all five subagents re-read the final two-comment set and returned
NO_NEW_VALUABLE_FINDINGS.
### What problem does this PR solve? Issue Number: None Related PR: apache#65500 Problem Summary: Corrupt external timestamp statistics with inverted bounds could terminate the BE, and a slotless unsafe conjunct could be omitted from localized table filters so a later constant predicate incorrectly pruned the split. Treat inverted statistics as unusable and track the constant-pruning safe prefix from the original conjunct order, including slotless ordering barriers. ### Release note Corrupt timestamp statistics and unsafe slotless predicates now fall back to conservative scan behavior. ### Check List (For Author) - Test: Regression test / Unit Test - TimestampStatisticsTest.* - TableReaderTest.ConstantPruningStopsAtUnsafePredicate - TableReaderTest.ConstantPruningStopsAtUnsafeSlotlessPredicate - Remote BE build - Behavior changed: Yes (split pruning now falls back conservatively for corrupt timestamp statistics and unsafe slotless predicates) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Automated review complete. I found five blocking correctness/test issues:
- file-local predicate localization can still cross slotless unsafe conjuncts (inline)
- inverted Parquet min/max stats are still trusted for pruning and native aggregate pushdown (inline)
- timestamp raw endpoint inversion and TIMESTAMPTZ bypasses can still trust corrupt stats (inline)
- the new slotless TableReader unit test asserts a predicate path this harness never executes (inline)
- MIN/MAX aggregate pushdown remains unsafe through non-order-preserving file-to-table mappings (inline)
Checkpoint conclusions:
- Goal/tests: the PR targets predicate/statistics hardening, but the changed paths still leave correctness gaps.
- Scope/focus: changes are focused, but safety barriers and aggregate/stat validation are incomplete in parallel execution paths.
- Concurrency/lifecycle/config/persistence: no new concurrency, lifecycle, config, persistence, or FE/BE protocol concerns found in the reviewed changed paths.
- Compatibility/data correctness: data correctness issues remain in pruning and aggregate pushdown; no storage-format compatibility change found.
- Tests: review-only environment; no builds or tests run here. One changed test appears to assert an impossible path in its harness.
- User focus: no additional user-provided focus.
Convergence: main review plus three real subagent rounds completed; the final round returned NO_NEW_VALUABLE_FINDINGS.
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
### What problem does this PR solve? Issue Number: None Related PR: apache#65500 Problem Summary: File-local predicate localization could cross a slotless unsafe conjunct, inverted Parquet and ORC statistics could still drive pruning or aggregate results, and MIN/MAX pushdown could use endpoints from a non-order-preserving file-to-table projection. Stop localization at the original conjunct-order barrier, reject inverted decoded and raw timestamp bounds, and require a direct mapping without a projection for MIN/MAX pushdown. Add focused coverage for inverted statistics, ordering barriers, and cast fallback. ### Release note Prevent incorrect external file pruning and MIN/MAX results from corrupt statistics, unsafe predicate reordering, and non-order-preserving schema mappings. ### Check List (For Author) - Test: Unit Test - Remote ASAN BE unit tests: 4 focused predicate/statistics/mapping tests passed - Remote ASAN BE unit tests: 13 Parquet, ORC, and timestamp statistics tests passed - Remote clang-tidy on the six modified files - Behavior changed: Yes (unsafe metadata and mappings now fall back to normal scanning) - Does this need documentation: No
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
Author
|
/review |
Contributor
|
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. |
Contributor
TPC-H: Total hot run time: 29589 ms |
Contributor
TPC-DS: Total hot run time: 179920 ms |
Contributor
ClickBench: Total hot run time: 25.05 s |
### What problem does this PR solve? Issue Number: None Related PR: apache#65500 Problem Summary: Direct VARBINARY mappings localized late runtime filters into external file readers even though those readers intentionally do not support VARBINARY predicate pushdown. This could reject matching Iceberg BINARY partition rows. Keep VARBINARY filters for scanner-level evaluation after materialization. ### 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.* (5 passed) - Behavior changed: Yes (VARBINARY runtime filters stay on scanner-level evaluation path) - Does this need documentation: No
### 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
### 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
### 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
### What problem does this PR solve? Issue Number: None Related PR: apache#65500 Problem Summary: Combining the Parquet reader changes exposed that direct file mappings carry a slot-ref materialization projection. Treating every projection as unsafe disabled valid MIN/MAX statistics pushdown and made existing direct-mapping tests return raw rows. Use mapping triviality as the safety boundary: direct slot-ref mappings remain eligible while casts and other conversions still fall back to row scanning. ### Release note Fix MIN/MAX aggregate pushdown for direct external-file column mappings. ### Check List (For Author) - Test: Unit Test - Remote ASAN BE unit tests: ColumnMapperLocalizeFiltersTest.*, TableReaderTest.*, and ParquetColumnReaderTest.* (121 passed) - Behavior changed: Yes (direct mappings retain MIN/MAX statistics pushdown; converted mappings still fall back) - Does this need documentation: No
Gabriel39
changed the title
Contributor
Author
|
run buildall |
Contributor
Author
|
/review |
Contributor
There was a problem hiding this comment.
Static review only: this checkout is missing .worktree_initialized, thirdparty/installed, and thirdparty/installed/bin/protoc, so I did not run builds or tests.
I reviewed the prompt-specified scope, existing review threads/comments, changed-file list, authoritative diff, and the shared subagent ledger. After accepting these four findings and rerunning final convergence, all live final agents returned NO_NEW_VALUABLE_FINDINGS for the same final comment set.
Checkpoint conclusions: the PR is focused on external-reader predicate/statistics/nested-reader correctness, but the remaining issues below can still produce wrong scan or aggregate results. I did not find a separate concurrency, lifecycle, config, persistence, transaction, FE/BE protocol, or observability issue. The main parallel-path gaps are ORC non-timestamp stats and complex-root VARBINARY predicate localization. The added tests were reviewed statically only; they do not cover the four residual failures called out inline.
| // escape before the filter arrives and cannot later be reconstructed from real file rows. | ||
| // This is the same irreversibility constraint as table-level metadata COUNT, and applies | ||
| // to COUNT and MIN/MAX for Parquet/ORC as well as COUNT for text readers. | ||
| if (!_all_runtime_filters_applied_for_split) { |
Contributor
There was a problem hiding this comment.
This still allows aggregate pushdown to reduce rows before an original row-level predicate has run. _build_table_filters_from_conjuncts() intentionally omits slotless conjuncts, so a scan with MIN/MAX(v) and a slotless unsafe predicate like random()/uuid() can reach this new RF gate with no pending filters and then pass the later _table_filters.empty() check. The reader then emits synthetic extrema rows and closes before Scanner::_filter_output_block() evaluates the original predicate, so the predicate runs on {min,max} instead of on every source row. For rows {1,2,3}, if the slotless predicate keeps only the middle row, normal scanning aggregates 2, but pushdown can aggregate {1,3}. Please disable aggregate pushdown whenever the original _conjuncts is non-empty, or otherwise prove every original conjunct has been represented by a safe, row-preserving pre-execution path before emitting synthetic COUNT/MIN/MAX rows.
| // `value = 1.0` is false. Complex Field equality also does not compare nested contents. | ||
| // Restrict localization to scalar numeric casts that preserve every file value; unsupported | ||
| // and complex casts keep the table predicate and evaluate after materialization. | ||
| if (!is_lossless_file_to_table_numeric_cast(rewrite_info.file_type, |
Contributor
There was a problem hiding this comment.
This numeric-only guard leaves TIMESTAMPTZ scale-mismatch predicates to the generic slot rewrite path, which then wraps the file slot in Cast(file_slot AS table_type). That conflicts with the mapping contract added for TIMESTAMPTZ scale mismatch: finalization treats TIMESTAMPTZ(6) -> TIMESTAMPTZ(0) as pass-through because the SQL cast rounds fractional seconds. For example, a file value 12:34:56.789 and predicate ts < '12:34:57' should remain true under pass-through semantics, but a pushed file-reader cast to TIMESTAMPTZ(0) can round the value to 12:34:57 and filter it before the scanner evaluates the original predicate. Please keep these scale-mismatch TIMESTAMPTZ filters above the file reader, or add a no-rounding file-local rewrite that matches final materialization semantics.
| // FileScanOperator deliberately keeps VARBINARY predicates above external readers. Their | ||
| // 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) { |
Contributor
There was a problem hiding this comment.
This guard only protects top-level VARBINARY mappings. Predicates rooted at a complex column are admitted by localize_filters() based on the root mapping, so nested VARBINARY can still reach the file reader. For example, element_at(s, 'bin') = ... rewrites the struct selector/type to the file child without checking the child mapping's filter_conversion; similarly, generic complex-root predicates such as array_contains(array<varbinary>, ...) or map_contains_key(map<varbinary,...>, ...) rewrite the root slot directly when the root mapping has a local source. That sends nested VARBINARY comparisons into the same file-reader expression/metadata paths this guard is trying to avoid for top-level VARBINARY. Please reject localization for any complex-root predicate whose referenced nested mapping is FINALIZE_ONLY, or at least whose nested table type contains VARBINARY.
| std::pair(timestamp_statistics->getMinimum(), timestamp_statistics->getMinimumNanos()); | ||
| const auto max_endpoint = | ||
| std::pair(timestamp_statistics->getMaximum(), timestamp_statistics->getMaximumNanos()); | ||
| if (min_endpoint > max_endpoint) { |
Contributor
There was a problem hiding this comment.
This adds conservative validation for timestamp stats only, but ORC MIN/MAX pushdown also consumes the sibling non-timestamp stripe stats through build_zone_map_from_orc_statistics(). The integer, floating, string, date, and decimal setters still return usable ZoneMaps without checking max_value < min_value, and the floating path also trusts NaN bounds. With corrupt ORC stats such as INT min=10/max=1 for a stripe that actually contains 5, aggregate pushdown can return synthetic extrema {10,1} instead of falling back to a real scan. Please apply the same conservative validation to every ORC statistics-backed min/max path, including rejecting NaN floating bounds and decoded inverted ranges, before allowing pruning or aggregate pushdown to trust the stats.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: Supersedes #65471, #65489, #65490, #65493, #65494, #65497, #65498, and #65499.
Problem Summary: FileScannerV2 and external file readers had several independent correctness and resource-control gaps:
The fix refreshes conjuncts per split, gates irreversible aggregate shortcuts on runtime-filter completion, keeps VARBINARY filters on the scanner-level path, preserves only the safe conjunct prefix, requires lossless predicate localization, and makes invalid or inexact statistics fall back conservatively. It also scopes Bloom filters correctly, bounds nested skip work in reusable batches, avoids materializing skipped nested values where possible, resets binary builders between level reads, and permits MIN/MAX pushdown only for order-preserving trivial mappings.
Release note
Fix incorrect external-file filtering, partition and metadata pruning, aggregate pushdown, and nested Parquet skip behavior. Runtime filters now use current split state, unsafe statistics paths fall back conservatively, and nested skips use bounded memory.
Check List (For Author)
gabriel@10.26.20.3:ColumnMapperLocalizeFiltersTest.*,TableReaderTest.*, andParquetColumnReaderTest.*(121 tests passed).git diff --checkpassed.stddef.hand pre-existing diagnostics outside these changes.