morningman opened a new pull request, #69821:
URL: https://github.com/apache/doris/pull/69821
### What problem does this PR solve?
Issue Number: None
Related PR: #65851, #69503
Problem Summary:
**In short.** When the FE sends no Iceberg scan semantics version (Doris
4.1.3 and earlier, for example during a rolling upgrade that upgrades the BEs
first), the V1 reader fills an equality-delete key that an older data file
lacks with a `ColumnConst` NULL. A composite key with a non-byte column then
fails the query, and a nested key in a data file without field IDs aborts the
BE in debug builds and reads the wrong memory in release builds. This PR builds
that NULL as a full column.
**Background.** A data file written before a key column was added has no
such column, and Iceberg reads it as the field's initial default, or NULL. In
the V1 reader (an FE this old never asks for `FileScannerV2`),
`IcebergReaderMixin::_create_missing_equality_delete_value` builds that value
once per file as a one-row column, and the reader repeats it for each batch
with `iceberg::repeat_initial_default_column` (`clone_empty()` +
`insert_many_from()`): in `_materialize_missing_equality_delete_column` for a
top-level key, and in `_extract_nested_equality_delete_column` for a key under
a struct that the file has. Without scan semantics v2 the FE sends no initial
defaults either, and the function returns the NULL as `create_column_const(1,
Field())`. The reader used to expand it with
`clone_resized(rows)->convert_to_full_column_if_const()`; #65851 switched to
`repeat_initial_default_column`, which keeps a `ColumnConst` a `ColumnConst`.
Everything after it reads the key row by row: `Mu
ltiEqualityDelete` hashes each key column with `update_hashes_with_value` and
confirms a hit with `compare_at`, and the nested extraction copies the leaf
with `insert_from(*current, row)`.
**The problem, and what it cost.** From the new unit tests, run against
master's reader:
| Plan without a scan semantics version, key missing from the data file |
Master today | After this PR |
|---|---|---|
| composite key with an INT column, e.g. `(id, added_int)` | the query
fails: `[E3] Method update_hashes_with_value is not supported for
Const(Nullable(INT))` (Parquet and ORC, with and without a lazy filter) | the
key is NULL: `(2, NULL)` deletes row 2, `(3, 7)` keeps row 3 |
| key `payload.k` under a `payload` struct that the file has, resolved by
name because the file has no field IDs | debug builds abort: `Bad cast from
type:doris::ColumnConst to doris::ColumnVector<(doris::PrimitiveType)5>`
(Parquet and ORC); release builds skip that check
(`TypeCheckOnRelease::DISABLE`) and read the `ColumnConst` object as INT values
(from the code, not run) | the key is NULL: `(1, NULL)` deletes row 1, `(3, 7)`
keeps row 3 |
| composite key with a STRING column | correct, through a `ColumnConst`
branch that #69503 added to the byte-key path | correct, without that branch |
| a single key | correct: `SimpleEqualityDelete` calls
`convert_to_full_column_if_const()` | unchanged |
The nested case breaks single keys as well, because the extraction runs
before either delete implementation.
**How this PR fixes it.**
- `_create_missing_equality_delete_value` builds the NULL as a one-row
nullable column (`create_column()` + `insert(Field())`), like the initial
defaults it builds on the other path, so every repeated key is a full column of
NULLs.
- `repeat_initial_default_column` asserts that its input is not a
`ColumnConst`.
- The byte-key path of `equality_delete.cpp` drops the `ColumnConst` branch
that #69503 added for this case and asserts that it gets no `ColumnConst`.
`EqualityDeleteTest.CompositeConstNullStringKeyMatchesNullDeleteKey` becomes
`CompositeNullStringKeyMatchesNullDeleteKey`, with a full NULL column.
branch-4.1 and branch-4.2 build and repeat the missing key the same way
(`be/src/format/table/iceberg_reader.cpp`).
**Results.**
- New BE unit tests in
`be/test/format/table/iceberg/iceberg_reader_test.cpp`, all for plans without a
scan semantics version:
- `v1_legacy_plan_multi_equality_delete_matches_missing_key_as_null`:
missing INT and STRING columns of composite keys, against NULL and non-NULL
delete keys.
- `v1_parquet_legacy_plan_missing_equality_key_reads_null` and
`v1_orc_legacy_plan_missing_equality_key_reads_null`: a missing top-level key
read end to end, with and without a lazy filter.
- `v1_legacy_plan_missing_nested_equality_key_reads_null`: the nested key,
Parquet and ORC.
With master's reader all four fail, the nested one by aborting the test
process. With this PR they pass, and so do the Iceberg, equality-delete and
Parquet reader suites (411 tests, filter
`*Iceberg*:*EqualityDelete*:ParquetReadLinesTest.*:SchemaTest.*:PartitionColumnFillerTest.*`).
- No regression test: the case needs an FE that sends no scan semantics
version in front of this BE.
### Release note
Fix: While the FEs are older than Doris 4.1.4, for example during a rolling
upgrade, reading an Iceberg table with the V1 file scanner no longer fails, or
crashes the BE, when an equality-delete key is missing from an older data file.
### Check List (For Author)
- Test <!-- At least one of them must be included. -->
- [ ] Regression test
- [x] Unit Test
- [ ] Manual test (add detailed scripts or steps below)
- [ ] No need to test or manual test. Explain why:
- [ ] This is a refactor/code format and no logic has been changed.
- [ ] Previous test can cover this change.
- [ ] No code files have been changed.
- [ ] Other reason <!-- Add your reason? -->
- Behavior changed:
- [x] No.
- [ ] Yes. <!-- Explain the behavior change -->
- Does this need documentation?
- [x] No.
- [ ] Yes. <!-- Add document PR link here. eg:
https://github.com/apache/doris-website/pull/1214 -->
### Check List (For Reviewer who merge this PR)
- [ ] Confirm the release note
- [ ] Confirm test cases
- [ ] Confirm document
- [ ] Add branch pick label <!-- Add branch pick label that this PR should
merge into -->
--
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]
