[
https://issues.apache.org/jira/browse/IMPALA-15375?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18117880#comment-18117880
]
ASF subversion and git services commented on IMPALA-15375:
----------------------------------------------------------
Commit 71c347b55997a674d557fbdcb18ec7e79861704f in impala's branch
refs/heads/master from Zoltan Borok-Nagy
[ https://gitbox.apache.org/repos/asf?p=impala.git;h=71c347b55 ]
IMPALA-15375: Materialize the file position slot before evaluating conjuncts
With late materialization only the readers whose slots a conjunct or runtime
filter reads are materialized before the conjuncts are evaluated; the rest
are materialized afterwards, for the surviving rows only.
The file position slot has no reader of its own. CreateColumnReaders()
attaches it to (*column_readers)[0], whichever reader that is. The split
only looked at each reader's own slot, so a conjunct on FILE__POSITION did
not move that reader into the filter group. When it was a non-filter reader
the conjunct was evaluated against a slot that had not been written yet,
reading whatever InitTupleBuffer() left there.
Rows went missing silently:
set parquet_late_materialization_threshold=1;
select string_col, file__position from alltypes_tiny_pages
where id = 6285 and file__position = 7000;
returned no rows, against one with late materialization disabled. The
Iceberg _row_id virtual column is affected the same way on V3 tables, as it
is rewritten to an expression over FILE__POSITION.
Treat the reader that writes the slot as a filter reader whenever a conjunct
reads it. That is always safe, since filter readers are simply materialized
for every row. If the reader is a collection it becomes filter_readers_[0]
and the existing max_rep_level() check falls back to
AssembleRowsWithoutLateMaterialization.
Testing:
- Added two queries to virtual-column-file-position-parquet.test, which
runs with parquet_late_materialization_threshold in {-1, 1, 17}. The
second keeps another non-filter column so late materialization stays
enabled. Both fail without the fix.
- Ran test_scanners.py -k "virtual_column or mixing" and
test_parquet_late_materialization.py.
Change-Id: I13fc71b5f81088e7efd1d64b8828b30268c40fb8
Assisted-by: Claude Opus 5 (1M context) <[email protected]>
Reviewed-on: http://gerrit.cloudera.org:8080/24887
Reviewed-by: Impala Public Jenkins <[email protected]>
Tested-by: Impala Public Jenkins <[email protected]>
> Conjunct on FILE__POSITION is evaluated before the slot is materialized when
> late materialization is used
> ---------------------------------------------------------------------------------------------------------
>
> Key: IMPALA-15375
> URL: https://issues.apache.org/jira/browse/IMPALA-15375
> Project: IMPALA
> Issue Type: Bug
> Components: Backend
> Reporter: Zoltán Borók-Nagy
> Assignee: Zoltán Borók-Nagy
> Priority: Major
>
> Under Parquet late materialization, a scan conjunct on the FILE__POSITION
> virtual column can be evaluated against a scratch tuple whose file-position
> slot has not been written yet, so the predicate sees 0 (or stale bytes)
> instead of the real position. The query then silently returns too few rows.
> *Status: derived by tracing the code, not yet reproduced at runtime.* Repro
> below.
> h2. Root cause
> The file-position slot is filled by {{(*column_readers)[0]}}
> (hdfs-parquet-scanner.cc:3021-3024),
> whichever column that happens to be.
> {{DivideFilterAndNonFilterColumnReaders()}}
> (hdfs-parquet-scanner.cc:274-291) classifies a reader by its own
> {{slot_desc()->id()}} only:
> {code:java}
> auto slot_desc = column_reader->slot_desc();
> if (slot_desc != nullptr && std::find(conjunct_slot_ids_.begin(),
> conjunct_slot_ids_.end(),
> slot_desc->id()) != conjunct_slot_ids_.end()) {
> filter_readers->push_back(column_reader);
> } else {
> non_filter_readers->push_back(column_reader);
> }
> {code}
> A conjunct on the *file-position* slot therefore does not make the reader
> that fills it a filter reader. If that reader ends up in
> {{non_filter_readers_}}, {{AssembleRows()}} runs the filter pass and
> {{FilterScratchBatch()}} before the slot is ever written.
> {{InitTupleBuffer()}}
> (hdfs-scanner.h:637) zeroes tuples of at most 64 bytes and clears only the
> null bits for larger ones, so the predicate reads 0 or stale bytes.
> h2. Repro (scalar, pre-existing)
> {{alltypes_tiny_pages}} is not a bulk-loaded table; create it as
> {{test_virtual_column_file_position_parquet}} does, with
> {{create_table_from_parquet(self.client, db, 'alltypes_tiny_pages')}} from
> {{testdata/data/alltypes_tiny_pages.parquet}}.
> {code:sql}
> set parquet_late_materialization_threshold=-1; -- control: expected 1 row
> select string_col, file__position from alltypes_tiny_pages
> where id = 6285 and file__position = 7000;
> set parquet_late_materialization_threshold=1; -- predicted: 0 rows
> select string_col, file__position from alltypes_tiny_pages
> where id = 6285 and file__position = 7000;
> {code}
> Here {{string_col}} is {{column_readers_[0]}} and a non-filter reader, while
> {{id}} is the filter
> reader. (The id/position pairing is taken from
> {{QueryTest/virtual-column-file-position-parquet.test:3-16}}.)
> h2. Why this matters now
> It becomes a *new* regression for VARIANT as soon as IMPALA-15142 lifts the
> late-materialization exclusion for variant readers. This query is correct
> today only because the variant forces
> {{AssembleRowsWithoutLateMaterialization()}}:
> {code:sql}
> select v, file__position from functional_parquet.trino_variant
> where id = 31 and file__position > 0;
> -- today: 1 row (fallback path)
> -- after IMPALA-15142 without this fix: predicted 0 rows
> {code}
> The same applies to {{_row_id}} on Iceberg V3 tables:
> {{IcebergVirtualColumnRewriteRule}}
> rewrites it to {{COALESCE(_file_row_id, ICEBERG__FIRST__ROW__ID +
> FILE__POSITION)}}, and every
> VARIANT table is a V3 table.
> Iceberg position-delete and deletion-vector plans are *not* affected: they
> are LEFT ANTI joins, so their predicates are join conjuncts and they get no
> runtime filters.
> h2. Suggested fix
> Treat the reader that owns the file-position slot as a filter reader when
> that slot is
> referenced by a conjunct. About five lines in
> {{DivideFilterAndNonFilterColumnReaders()}}, using
> the existing accessor {{ParquetColumnReader::file_pos_slot_desc()}}
> (parquet-column-readers.h:87).
> Note the classification runs at hdfs-parquet-scanner.cc:260, after
> {{CreateColumnReaders()}} has attached the slot, so the information is
> available.
> Forcing a reader into {{filter_readers_}} is safe: if it is a collection it
> lands at
> {{filter_readers_[0]}} and the existing {{max_rep_level() > 0}} guard falls
> back to the non-LM
> path; a variant as a filter reader already works today. Falling back to
> {{AssembleRowsWithoutLateMaterialization()}} whenever a file-position
> conjunct exists would be an even simpler, more conservative alternative.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
