u70b3 commented on PR #66514: URL: https://github.com/apache/doris/pull/66514#issuecomment-5199892256
Thanks for the reviews — all points are addressed in 961e9332102 (just pushed). **Pattern A validation (P1)** — `_decode_values` now validates the decoded index stream before entering either decoding branch, mirroring `decode_dictionary_indices` in `format_v2/parquet/reader/native/decoder.h`: `GetBatch` must return exactly `non_null_size` indices (truncated stream → `Status::Corruption`), and every index must be `< _dict_items.size()` (out-of-range → `Status::Corruption`). This subsumes the previous empty-dictionary-only check and also covers the dictionary-column path, whose raw indices previously flowed unvalidated into `convert_dict_column_to_string_column`. **Tests (P2 / @Gabriel39 point 1)** — direct regression coverage added: - decoder level: empty dictionary + non-null data, out-of-range index (plain and dictionary-column paths), truncated index stream — all assert `Status::Corruption` (`byte_array_dict_decoder_test.cpp`); - reader level: `IcebergParquetReader` init with FE schema info missing a projected column asserts `Status::InternalError` (`parquet_init_fails_loudly_when_schema_mapping_misses_projected_column`); - optimization path: page-index filtering with a synthetic TopN `GLOBAL_ROWID_COL` slot absent from the FE-built schema tree asserts the optimization is skipped and the full row-group range is kept (`test_page_index_filter_skips_synthetic_slot_absent_from_schema_mapping`); - "known nested field absent from the file still materializes its default/NULL at reader level" was already covered by `v2_parquet_materializes_nested_initial_default_without_reviving_parent` and `v1_top_level_missing_binary_prefers_iceberg_initial_default`. All 130 tests in the touched suites pass locally. **Issue closing (@Gabriel39 point 2)** — removed `close #61225` from the description. Pattern A is now guarded at the crash site (every SIGSEGV mode at that stack frame returns `Corruption`), but the upstream trigger remains unproven without a reproducer, so the issue stays open. **Scope (P1)** — kept as one PR because all guarded sites share a single mechanism (bare `StructNode::children.at()`), and the new synthetic-slot test demonstrates a reachable path that needs no FE misbehavior. A centralized safe-lookup API to replace the repeated `has_children_column() + children_column_exists()` pairs is a good follow-up; this PR intentionally keeps the cbfe3096dff precedent pattern for consistency. -- 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]
