[fix](parquet) Restrict V2 dictionary filtering to strings - #66343
[fix](parquet) Restrict V2 dictionary filtering to strings#66343Gabriel39 wants to merge 2 commits into
Conversation
|
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.
The string-only gate is in the correct file-local domain and the decoded/direct/residual fallback is selected before cursor creation, so no query-correctness or lifecycle defect was found. Changes are requested because the patch lacks a result/error regression for its claimed correctness failure and leaves fixed-width dictionary Profile/benchmark machinery unreachable but documented as active. No additional user-provided focus was present; the full PR was reviewed.
| } | ||
|
|
||
| TEST_F(ParquetScanTest, PredicateOnlyDictionaryRangeSkipsTypedValueMaterialization) { | ||
| TEST_F(ParquetScanTest, PredicateOnlyNonStringDictionaryRangeUsesDecodedValues) { |
There was a problem hiding this comment.
The renamed INT/BIGINT tests only invert Profile expectations; their row counts and values already pass on the parent, so they do not reproduce the stated logical-domain correctness bug. Please add a reachable non-identity case (for example mapper/schema evolution or date/decimal/unsigned/fixed-binary) where the old dictionary-ID path returns wrong rows or the wrong error, and assert parity with V1 or the decoded fallback. Otherwise this blanket non-string restriction has no result-level regression proving why the performance path must be disabled.
| column_schema.max_repetition_level > 0) { | ||
| return false; | ||
| } | ||
| if (!is_string_type(remove_nullable(column_schema.type)->get_primitive_type())) { |
There was a problem hiding this comment.
This guard rejects every primitive handled by get_typed_dictionary_raw_values() before build_dictionary_entry_filter() runs. As a result, TYPED_FIXED_WIDTH, the fixed-width fused projection path, DictFilterTypedCompareColumns, and DictionaryPredicateFusedProjectedRows now have no reachable producer, while the design metrics table and INT64 benchmark matrix still advertise them. Please remove/update the dead machinery and counters, or retain a semantics-safe reachable producer, so Profiles and benchmark coverage describe executable behavior.
|
run buildall |
|
/review |
There was a problem hiding this comment.
Three new blocking/correctness findings remain:
- The new fixed-length dictionary expansion can amplify a roughly 1 MiB schema-accepted Parquet file into a 1 GiB scratch allocation before DECIMAL conversion rejects or nulls the oversized width.
- The whole-chunk encoding whitelist admits mixed PLAIN/dictionary
FIXED_LEN_BYTE_ARRAYfor raw-binary-only string predicates, so a valid PLAIN-first chunk makes progress and then fails as corruption on its dictionary page. - Projected direct filtering can append early survivors and then return a late dictionary/consumer error without restoring the caller-owned output column.
Critical checkpoint conclusions:
- Native boundary and ownership: the change remains inside the V2 native Parquet path; no new Arrow/V1 fallback or cross-layer mapping dependency was introduced.
- Filtering and conversion: successful numeric, DATE/TIME/timestamp, DECIMAL, FLOAT16, nullable, strict/permissive, IN/Bloom/TopN, selection, and cursor paths otherwise preserve the checked logical SerDe domain.
- Lifecycle, corruption, and performance: the three inline findings cover the substantiated late-fallback, output-rollback, and byte-unbounded scratch failures.
- Tests and observability: the two existing live threads already cover the dead typed-dictionary metrics/design claims and the missing non-identity result-level regression, so they were not duplicated.
- User focus: no additional user-provided focus was supplied; the complete PR was reviewed.
- Review status: static review only as required; no builds were run. Two convergence rounds completed, and every normal and risk-focused subagent returned
NO_NEW_VALUABLE_FINDINGSagainst this frozen comment set. The review is complete with three new inline findings.
The prompt-referenced repository code-review SKILL.md was not present in this checkout; the required be/src/format_v2/AGENTS.md and all mandatory linked design/checklist documents were applied.
| if (UNLIKELY(num_values > std::numeric_limits<size_t>::max() / _value_width)) { | ||
| return Status::IOError("Parquet dictionary expansion size overflows"); | ||
| } | ||
| _scratch.resize(num_values * _value_width); |
There was a problem hiding this comment.
[P1] This expansion is bounded by values, not bytes. FIXED_LEN_BYTE_ARRAY accepts any positive type_length, and DECIMAL validation only checks that the declared precision fits that width. A required DECIMAL(1,0) with a 1 MiB width, a one-entry dictionary, and a 1,024-row repeated-ID page is only slightly over 1 MiB on disk, but consume_repeated() resizes this scratch to exactly 1 GiB before Decimal SerDe rejects widths above Int256 (or marks them NULL in permissive mode). The fragmented/indices path can amplify further. Please validate the logical/physical width before selecting this path and byte-bound or chunk the expansion, with an oversized FLBA dictionary regression for strict and permissive scans.
| ColumnChunkReader<IN_COLLECTION, OFFSET_INDEX>:: | ||
| supports_raw_binary_filter_encoding( | ||
| encoding, _chunk_meta.meta_data.type) || | ||
| ColumnChunkReader<IN_COLLECTION, OFFSET_INDEX>:: |
There was a problem hiding this comment.
[P1] This chunk-level whitelist is broader than the page-level predicate capability. For a logical string backed by FIXED_LEN_BYTE_ARRAY, a comparison can run only through the raw-binary consumer: PLAIN/BYTE_STREAM_SPLIT pages are accepted, but RLE/PLAIN_DICTIONARY pages are not, and String SerDe has no converted-fixed predicate path. Because this new clause makes metadata containing both encodings pass the up-front check, a valid PLAIN-first/dictionary-later chunk consumes the first page and then returns used_filter=false; NativeColumnReader converts that late fallback into corruption. Please reject dictionary encodings up front for raw-binary-only conjuncts (or add a dictionary binary consumer), with a regression covering that page order.
| std::vector<uint8_t>& _scratch; | ||
| } expanded_consumer(_dict.get(), _num_dictionary_values, static_cast<size_t>(_type_length), | ||
| consumer, _expanded_values); | ||
| return decode_selected_dictionary_values(selection, expanded_consumer); |
There was a problem hiding this comment.
[P2] This streams directly into FixedWidthPredicateConsumer, which appends matching values to the caller-owned projected column for each validated range/run. A later selected ID, filtered-tail ID, or consumer conversion can still fail after those early appends, and neither this method nor filter_fixed_width_values() restores the column. The ordinary dictionary gather path explicitly snapshots and resize(old_size) on the same late-corruption case (DictionaryDirectGatherRollsBackLateCorruptRun). Please add the same all-or-nothing rollback around projected direct filtering and cover a valid selected head followed by a corrupt filtered tail or later run.
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
2 participants
What problem does this PR solve?
File Scanner V2 applies row-level dictionary-ID filtering to non-string Parquet dictionaries. This differs from the V1 and StarRocks strategy, where non-string dictionary values are decoded before filtering, and can bypass the intended logical value domain.
What is changed?
Verification