[
https://issues.apache.org/jira/browse/HIVE-30143?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
László Bodor updated HIVE-30143:
--------------------------------
Description:
h2. Background
HIVE-30059 added a native Parquet read path to LLAP IO: it caches column-chunk
bytes and
decodes them with the same {{VectorizedColumnReader}}s the stock reader uses.
The decode
step is shared (via {{ParquetRowGroupDecoder}}), but everything leading up to
it is not.
Each reader independently derives the same per-file facts from the same two
inputs -- the
Parquet footer and the job conf -- in its own place.
h2. The duplication
||Decision||Stock reader||Native reader||
|requested schema|{{VectorizedParquetRecordReader.initialize}}
:270-272|{{ParquetEncodedDataReader.readFooter}} :196|
|timestamp / proleptic / legacy-zone flags|{{ParquetRecordReaderBase.getSplit}}
:160-170|{{ParquetEncodedDataConsumer.initSchema}} :131-144|
|writer timezone|{{VectorizedParquetRecordReader}}
:266-267|{{ParquetEncodedDataConsumer.initSchema}} :143|
|row groups in the split (first-data-page
rule)|{{ParquetRecordReaderBase.getSplit}}
:130-140|{{ParquetEncodedDataReader.selectRowGroups}} :293-303|
|SARG to {{FilterPredicate}}, statistics
filter|{{ParquetRecordReaderBase.setFilter}} :144,
:189-205|{{ParquetEncodedDataReader.selectRowGroups}} :304-308|
The two sides agree today. The problem is that they agree by coincidence of
maintenance
rather than by construction: each of these five decisions has two independent
implementations that must be changed together, and nothing in the build or the
test suite
fails if only one of them is. A divergence would not throw -- it would return
wrong values
for some subset of files (a timestamp shifted by a zone, a row group read that
the
predicate had ruled out), visible only as a query answering differently
depending on
whether the data happened to be in the cache.
h2. Proposal
One class in {{ql}}, built once per split from inputs both readers already
hold, whose
fields are exactly those decisions:
{code:java}
/** Everything a Parquet split read decides once from the footer and the job
conf, shared by both readers. */
public final class ParquetSplitPlan {
public final MessageType requestedSchema;
public final List<BlockMetaData> rowGroups; // in-split, statistics-filtered
public final TimestampConversionOptions conversions;
public final FilterPredicate predicate; // may be null
public static ParquetSplitPlan of(JobConf conf, ParquetMetadata footer, long
splitStart, long splitLength) { ... }
}
{code}
h2. Resulting shape
* {{VectorizedParquetRecordReader.initialize}} and
{{ParquetEncodedDataReader.readFooter}}
both reduce to "read the footer, then {{ParquetSplitPlan.of(...)}}".
* {{ParquetEncodedDataConsumer.initSchema}} disappears -- the consumer is
handed the plan
instead of re-deriving it, which also removes its lazy {{schemaInitialized}}
latch.
* {{ParquetRecordReaderBase.toFilterPredicate}} and the five
{{DataWritableReadSupport}}
statics stop being called across the module boundary and go back behind the
plan. The
public surface between {{ql}} and {{llap-server}} becomes one type rather
than six
methods.
h2. Notes
* No behaviour change is intended. The decisions are the same; only the number
of places
that make them changes.
* Scoped as a follow-up deliberately: it touches the stock reader, which serves
every
Parquet query whether or not LLAP IO is on, so it should not ride along with
the native
path's own review.
* A sixth fact, the decimal64 physical variation, was on this list but has
since converged
-- both readers now take it from the plan's {{VectorizedRowBatchCtx}}
({{VectorizedParquetRecordReader:369}} via {{createVectorizedRowBatch}},
{{ParquetEncodedDataConsumer:128-129}} via
{{getRowdataTypePhysicalVariations}}). It is
listed here only so the history is not confusing.
* Regression net for the refactor already exists:
{{TestVectorizedColumnReader}},
{{TestVectorizedListColumnReader}}, {{TestVectorizedMapColumnReader}},
{{TestVectorizedDictionaryEncodingColumnReader}} (43 tests) cover the stock
side, and
{{TestParquetEncodedDataReader}} plus {{TestParquetCachedPageReadStore}} (49)
the native
side.
> Parquet: share one per-split read plan between the stock and native LLAP
> readers
> --------------------------------------------------------------------------------
>
> Key: HIVE-30143
> URL: https://issues.apache.org/jira/browse/HIVE-30143
> Project: Hive
> Issue Type: Improvement
> Reporter: László Bodor
> Priority: Major
>
> h2. Background
> HIVE-30059 added a native Parquet read path to LLAP IO: it caches
> column-chunk bytes and
> decodes them with the same {{VectorizedColumnReader}}s the stock reader uses.
> The decode
> step is shared (via {{ParquetRowGroupDecoder}}), but everything leading up to
> it is not.
> Each reader independently derives the same per-file facts from the same two
> inputs -- the
> Parquet footer and the job conf -- in its own place.
> h2. The duplication
> ||Decision||Stock reader||Native reader||
> |requested schema|{{VectorizedParquetRecordReader.initialize}}
> :270-272|{{ParquetEncodedDataReader.readFooter}} :196|
> |timestamp / proleptic / legacy-zone
> flags|{{ParquetRecordReaderBase.getSplit}}
> :160-170|{{ParquetEncodedDataConsumer.initSchema}} :131-144|
> |writer timezone|{{VectorizedParquetRecordReader}}
> :266-267|{{ParquetEncodedDataConsumer.initSchema}} :143|
> |row groups in the split (first-data-page
> rule)|{{ParquetRecordReaderBase.getSplit}}
> :130-140|{{ParquetEncodedDataReader.selectRowGroups}} :293-303|
> |SARG to {{FilterPredicate}}, statistics
> filter|{{ParquetRecordReaderBase.setFilter}} :144,
> :189-205|{{ParquetEncodedDataReader.selectRowGroups}} :304-308|
> The two sides agree today. The problem is that they agree by coincidence of
> maintenance
> rather than by construction: each of these five decisions has two independent
> implementations that must be changed together, and nothing in the build or
> the test suite
> fails if only one of them is. A divergence would not throw -- it would return
> wrong values
> for some subset of files (a timestamp shifted by a zone, a row group read
> that the
> predicate had ruled out), visible only as a query answering differently
> depending on
> whether the data happened to be in the cache.
> h2. Proposal
> One class in {{ql}}, built once per split from inputs both readers already
> hold, whose
> fields are exactly those decisions:
> {code:java}
> /** Everything a Parquet split read decides once from the footer and the job
> conf, shared by both readers. */
> public final class ParquetSplitPlan {
> public final MessageType requestedSchema;
> public final List<BlockMetaData> rowGroups; // in-split, statistics-filtered
> public final TimestampConversionOptions conversions;
> public final FilterPredicate predicate; // may be null
> public static ParquetSplitPlan of(JobConf conf, ParquetMetadata footer,
> long splitStart, long splitLength) { ... }
> }
> {code}
> h2. Resulting shape
> * {{VectorizedParquetRecordReader.initialize}} and
> {{ParquetEncodedDataReader.readFooter}}
> both reduce to "read the footer, then {{ParquetSplitPlan.of(...)}}".
> * {{ParquetEncodedDataConsumer.initSchema}} disappears -- the consumer is
> handed the plan
> instead of re-deriving it, which also removes its lazy
> {{schemaInitialized}} latch.
> * {{ParquetRecordReaderBase.toFilterPredicate}} and the five
> {{DataWritableReadSupport}}
> statics stop being called across the module boundary and go back behind the
> plan. The
> public surface between {{ql}} and {{llap-server}} becomes one type rather
> than six
> methods.
> h2. Notes
> * No behaviour change is intended. The decisions are the same; only the
> number of places
> that make them changes.
> * Scoped as a follow-up deliberately: it touches the stock reader, which
> serves every
> Parquet query whether or not LLAP IO is on, so it should not ride along
> with the native
> path's own review.
> * A sixth fact, the decimal64 physical variation, was on this list but has
> since converged
> -- both readers now take it from the plan's {{VectorizedRowBatchCtx}}
> ({{VectorizedParquetRecordReader:369}} via {{createVectorizedRowBatch}},
> {{ParquetEncodedDataConsumer:128-129}} via
> {{getRowdataTypePhysicalVariations}}). It is
> listed here only so the history is not confusing.
> * Regression net for the refactor already exists:
> {{TestVectorizedColumnReader}},
> {{TestVectorizedListColumnReader}}, {{TestVectorizedMapColumnReader}},
> {{TestVectorizedDictionaryEncodingColumnReader}} (43 tests) cover the stock
> side, and
> {{TestParquetEncodedDataReader}} plus {{TestParquetCachedPageReadStore}}
> (49) the native
> side.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
