Zoltán Borók-Nagy created IMPALA-15608:
------------------------------------------
Summary: Iceberg MERGE crashes or modifies wrong rows when the ON
clause references target virtual columns
Key: IMPALA-15608
URL: https://issues.apache.org/jira/browse/IMPALA-15608
Project: IMPALA
Issue Type: Bug
Components: Backend, Frontend
Reporter: Zoltán Borók-Nagy
A MERGE into an Iceberg table whose ON clause references virtual columns of the
target table ({{_row_id}}, {{_last_updated_sequence_number}},
{{FILE__POSITION}}, {{PARTITION__SPEC__ID}}) hits a DCHECK in
IcebergMergeNode::AddRow() on debug builds. In one variant no DCHECK fires and
the statement deletes the wrong row.
Repro on master (debug build):
{code:sql}
create table t (i int) stored by iceberg tblproperties('format-version'='3');
insert into t values (1), (2), (3);
create table s (id bigint, v int) stored by iceberg
tblproperties('format-version'='2');
insert into s values (0, 100), (99, 999);
merge into t using s on t._row_id = s.id when matched then update set i = s.v;
{code}
{noformat}
F1021 tuple.cc:277] Check failed: slot_desc->type().type == TYPE_NULL ||
slot_desc->type() == evals[i]->root().type()
impala::Tuple::MaterializeExprs<>()
impala::IcebergMergeNode::AddRow()
impala::IcebergMergeNode::EvaluateCases()
impala::IcebergMergeNode::GetNext()
{noformat}
Other variants, each on freshly created t and s as above:
||ON clause||Target||Merge case||Result||
|t._row_id = s.id|V3|WHEN MATCHED THEN DELETE|DCHECK tuple.h:215 evals.size()
== desc.slots().size() (3 vs. 5)|
|t._row_id = s.id|V3|WHEN NOT MATCHED THEN INSERT|DCHECK tuple.h:215 (3 vs. 5)|
|t._last_updated_sequence_number = s.id + 1|V3|WHEN MATCHED THEN UPDATE|DCHECK
tuple.h:215 (5 vs. 7)|
|t.FILE__POSITION = s.id|V3, V2|WHEN MATCHED THEN UPDATE|DCHECK tuple.cc:277|
|t.FILE__POSITION = s.id|V2|WHEN MATCHED THEN DELETE|DCHECK tuple.cc:277|
|t.PARTITION__SPEC__ID = s.id and t.i = 1|V2|WHEN MATCHED THEN UPDATE|DCHECK
tuple.h:215 (3 vs. 4)|
|t.INPUT__FILE__NAME is not null and t._row_id = s.id + 1|V3|WHEN MATCHED THEN
UPDATE SET i = s.v|wrong result, no error|
The last statement should update the row with _row_id 1 (i = 2). Instead it
deletes the row with _row_id 0 (i = 1) and keeps i = 2, so _row_id 1 appears
twice:
{noformat}
select i, _row_id from t order by i;
2 1
3 2
100 1
{noformat}
(expected: 1/0, 3/2, 100/1)
These work correctly: INPUT__FILE__NAME alone in ON, and target virtual columns
in a WHEN MATCHED AND condition or in the SET expressions.
*Analysis*
IcebergMergeNode::AddRow() materializes the target tuple from
IcebergMergeCase::combined_evaluators_. That list holds the case's exprs for
the visible columns, then the row-meta exprs, then the partition-meta exprs:
* row-meta: INPUT__FILE__NAME, FILE__POSITION,
[ICEBERG__DATA__SEQUENCE__NUMBER], [_file_row_id, ICEBERG__FIRST__ROW__ID]
* partition-meta: PARTITION__SPEC__ID, ICEBERG__PARTITION__SERIALIZED
Tuple::MaterializeExprs() pairs these evaluators with the slots of the target
tuple by position, and the slots come in the order the frontend registered them.
IcebergMergeImpl.analyze() registers the visible target columns, then analyzes
the source table ref, including the ON clause. Only after that does it register
the row-meta and partition-meta slots, in IcebergMergeQueryGenerator.generate()
and in the analysis of the generated query. So a target virtual column that ON
references gets its
slot right after the visible columns. Two of them register more than one slot,
because they are rewritten:
* {{_row_id}} becomes {{COALESCE(_file_row_id, ICEBERG__FIRST__ROW__ID +
FILE__POSITION)}}.
* {{_last_updated_sequence_number}} becomes
{{COALESCE(_file_last_updated_sequence_number,
ICEBERG__DATA__SEQUENCE__NUMBER)}}.
What happens next depends on the statement:
* A slot gets an evaluator of a different type (tuple.cc:277).
* There are more slots than evaluators (tuple.h:215).
* If the swapped slots all have the same type, the values are swapped silently.
In the wrong-result example, the FILE__POSITION slot gets the
ICEBERG__FIRST__ROW__ID value (0), so the position delete hits the wrong row.
Release builds have no DCHECK. There, the mismatched cases put values of the
wrong type into the slots that feed the delete and insert sinks, or read past
the end of the evaluator vector. I did not test release builds.
The V3 variants became reachable with the row lineage columns (IMPALA-14592,
IMPALA-14825) and V3 MERGE (IMPALA-14937). The analysis order dates back to
IMPALA-12732, so the V2 FILE__POSITION / PARTITION__SPEC__ID variants probably
affect 4.5.x as well (not verified).
*Suggested fix*
Don't depend on slot registration order. Build each merge case's
materialization exprs from the target tuple's materialized slots: the case's
expr for visible columns, and a pass-through SlotRef for every other slot.
Registering the row-meta and partition-meta slots before analyzing ON would fix
only the reordering, not the extra slots,
e.g. _file_last_updated_sequence_number. IcebergMergeNode should also check the
slot/evaluator counts and types in Prepare() and return an error. Add tests
with target virtual columns in the MERGE ON clause, covering UPDATE, DELETE and
INSERT cases on V2 and V3 tables.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
