[
https://issues.apache.org/jira/browse/IMPALA-15373?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18118109#comment-18118109
]
ASF subversion and git services commented on IMPALA-15373:
----------------------------------------------------------
Commit a45095e75da4437cb1bfeb098149b2fa1e6249af in impala's branch
refs/heads/master from Zoltan Borok-Nagy
[ https://gitbox.apache.org/repos/asf?p=impala.git;h=a45095e75 ]
IMPALA-15373: Fix FILE__POSITION for complex readers in later row groups
ReadFilePositionNonBatched() returns 'row_group_first_row_ +
LastProcessedRow() + 1', but 'row_group_first_row_' was only assigned in
BaseScalarColumnReader::Reset(). The file position slot is attached to
(*column_readers)[0] whatever kind of reader that is, so when that is a
complex reader (collection, struct or VARIANT) the offset stayed 0 and
the second and later row groups reported row-group-relative positions.
This also mis-applies Iceberg position deletes, which match on
FILE__POSITION.
A regression from IMPALA-11780, which made 'current_row_'
row-group-relative and added the compensating term only on the scalar
path.
Pass the offset to ComplexColumnReader::Reset() as well, mirroring
InitScalarColumns(). complex_readers_ holds every complex reader,
including nested ones and the one CreateCountingReader() builds, so all
of them are covered. LastProcessedRow() stays row-group-relative, so
skip_row_id and the page index arithmetic are unaffected. ORC does not
share the bug.
Testing:
- Two expected results in virtual-column-file-position-parquet.test
encoded the old behaviour and are corrected to the true row indexes.
- Added regression queries over all row groups of two different files.
- The struct and VARIANT paths are fixed by the same change but have no
regression test, because no checked-in file has both a top level
struct or VARIANT column and more than one row group.
- Ran test_scanners.py -k test_virtual_column_file_position_parquet.
Change-Id: Ieb186c5727ae2a0cd53fb0c68ac540b8cd9fa8c7
Assisted-by: Claude Fable 5.1 (Claude Code)
Reviewed-on: http://gerrit.cloudera.org:8080/24885
Reviewed-by: Zoltan Borok-Nagy <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
> Wrong FILE__POSITION when the first materialized column is complex and the
> file has multiple row groups
> -------------------------------------------------------------------------------------------------------
>
> Key: IMPALA-15373
> URL: https://issues.apache.org/jira/browse/IMPALA-15373
> Project: IMPALA
> Issue Type: Bug
> Components: Backend
> Reporter: Zoltán Borók-Nagy
> Assignee: Zoltán Borók-Nagy
> Priority: Major
>
> FILE__POSITION is row-group-relative instead of file-relative whenever the
> Parquet column reader that fills the slot is a complex reader (collection,
> struct or VARIANT) and the data file has more than one row group. Rows in the
> second and later row groups get positions that are too small by the row-group
> offset.
> This is a regression introduced by IMPALA-11780.
> h2. Root cause
> IMPALA-11780 made {{current_row_}} row-group-relative and compensated for it
> where the file position is produced:
> {code:java}
> // parquet-column-readers.cc:1250, BaseScalarColumnReader::Reset()
> - current_row_ = row_group_first_row - 1;
> + row_group_first_row_ = row_group_first_row;
> + current_row_ = -1;
> // parquet-column-readers.h:738,
> ParquetColumnReader::ReadFilePositionNonBatched()
> - *file_pos = LastProcessedRow() + 1;
> + *file_pos = row_group_first_row_ + LastProcessedRow() + 1;
> {code}
> {{row_group_first_row_}} (parquet-column-readers.h:235) is a member of the
> {{ParquetColumnReader}} base, but it is assigned in exactly one place:
> {{BaseScalarColumnReader::Reset()}} (parquet-column-readers.cc:1250).
> {{ComplexColumnReader::Reset()}} (parquet-complex-column-reader.h:53-57) does
> not set it, and
> {{HdfsParquetScanner::InitComplexColumns()}} (hdfs-parquet-scanner.cc:1005,
> :3094) takes no
> row_group_first_row argument. A complex reader therefore keeps the default 0
> forever.
> {{ReadFilePositionNonBatched()}} is called on {{this}} from
> {{StructColumnReader::ReadValueBatch()}} and
> {{CollectionColumnReader::ReadValueBatch()}}, so
> for a complex reader the compensation term is always 0. Before IMPALA-11780
> this path was
> correct, because {{LastProcessedRow()}} delegates to {{children_[0]}}, whose
> {{current_row_}}
> was absolute at the time.
> The file-position slot is attached to {{(*column_readers)[0]}} regardless of
> reader kind
> (hdfs-parquet-scanner.cc:3021-3024). Slot order is analysis/registration
> order (FROM-clause
> refs, then the select list left to right, then WHERE), so the first reader is
> a complex one
> for queries such as {{select file__position, v from t}}, {{select v from t
> where id = 5}} or
> {{select ... from t c, c.c_orders}}.
> h2. The repo already contains the wrong values
> {{QueryTest/virtual-column-file-position-parquet.test:264-279}} has goldens
> for
> {{customer_nested_multiblock_multipage}} (300 rows in 3 row groups of 100).
> The only
> top-level reader in those queries is the {{c_orders}} CollectionColumnReader,
> so it owns the file-position slot.
> || query || golden || true file position ||
> | l_shipdate='1998-11-26' | 80 | 280 |
> | l_partkey = 199994 | 51, 82 | 151, 282 |
> The true positions were read back from
> {{testdata/data/customer_nested_multiblock_multipage.parquet}} with pyarrow.
> The goldens are exactly the true values minus the row-group offsets (100,
> 200, 200). They were added by
> c56cd7b214 itself, so the test has been locking in the bug since 4.3.0.
> h2. Impact
> * Wrong FILE__POSITION values returned to the user.
> * Wrong application of Iceberg V2 position deletes / V3 deletion vectors:
> IcebergScanPlanner
> adds FILE__POSITION to the data scan tuple and IcebergDeleteNode probes
> with it. Deletes
> aimed at row group 1 positions are also applied at the same relative
> offsets in later row
> groups, and deletes aimed at later row groups are missed.
> * Worst case, persistent corruption: a DELETE whose predicate only touches a
> complex or
> VARIANT column materializes only that column, so the complex reader owns
> the slot and
> wrong positions are written into the new position-delete file / DV.
> Only files with more than one row group are affected. Impala writes one row
> group per file,
> so this needs data written by Spark, Hive, Trino or Flink (Iceberg defaults:
> 512 MB target
> file, 128 MB row group, i.e. up to ~4 row groups per file).
> h2. Suggested fix
> Give complex readers the offset, e.g.:
> {code:java}
> // parquet-complex-column-reader.h
> void Reset(int64_t row_group_first_row) {
> def_level_ = rep_level_ = ParquetLevel::INVALID_LEVEL;
> pos_current_value_ = ParquetLevel::INVALID_POS;
> row_group_first_row_ = row_group_first_row;
> }
> // hdfs-parquet-scanner.{h,cc}: InitComplexColumns(int64_t
> row_group_first_row), passed at :1005
> {code}
> {{complex_readers_}} already holds every complex reader, including nested
> ones and the
> collection built by {{CreateCountingReader()}}, via the
> {{PartitionReaders()}} recursion.
> An alternative is to take the offset from {{children_[0]}} inside
> {{ReadFilePositionNonBatched()}}. Note that {{LastProcessedRow()}} must stay
> row-group-relative
> either way: it feeds {{skip_row_id}} (hdfs-parquet-scanner.cc:2442) and the
> page-index
> arithmetic in {{SkipRowsInternal()}}.
> The fix must also correct the goldens in
> virtual-column-file-position-parquet.test:268 and
> :275-276 to 280 and 151, 282, and state in the commit message that the old
> values were wrong.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
