[jira] [Commented] (IMPALA-15373) Wrong FILE__POSITION when the first materialized column is complex and the file has multiple row groups

["ASF subversion and git services (Jira)" <ji...@apache.org>]

    [ 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 <bo...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>


> 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: issues-all-unsubscribe@impala.apache.org
For additional commands, e-mail: issues-all-help@impala.apache.org