Gabriel39 commented on PR #66514:
URL: https://github.com/apache/doris/pull/66514#issuecomment-5199576162
Thanks for the defense-in-depth work. I have two concerns that should be
addressed before merge:
1. The new tests do not exercise the production fixes.
`HasChildrenColumnGuardsNestedStructField` and
`NestedStructFieldMissingInFileKeepsKey` only validate the existing
`has_children_column()` helper state. They would still pass if every new reader
guard and the `ByteArrayDictDecoder` change in this PR were removed. Please add
direct regression coverage for:
- an empty byte-array dictionary with non-null dictionary-encoded values
returning `Status::Corruption` instead of crashing;
- a Parquet/ORC reader schema-contract mismatch returning
`Status::InternalError`;
- a known nested field that is absent from the file still materializing
its default or NULL at reader level.
An SQL reproducer is not required for these cases; the existing
reader/decoder unit-test fixtures can construct the inconsistent state directly.
2. `close #61225` seems premature for Pattern A. The PR explicitly says that
the root cause is unproven and currently only guards the `_dict_items.empty()`
case. The reported stack location alone does not establish that this was the
exact state that caused the original SIGSEGV. Please either add a
reproducer/test that ties the reported failure to an empty dictionary, or
describe this part as hardening/partial mitigation and avoid automatically
closing the entire issue until the root cause is confirmed.
The `has_children_column()` short-circuit ordering, the distinction between
query errors and optimization fallback, and preservation of the
known-but-file-missing path otherwise look reasonable.
--
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]
