[
https://issues.apache.org/jira/browse/ORC-2223?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
yongzhi.shao updated ORC-2223:
------------------------------
Description:
Description:
h3. Problem
When scanning an ORC file, \{{RecordReaderImpl.beginReadStripe()}} already
reads the \{{OrcProto.StripeFooter}} for the current stripe (via
\{{readStripeFooter(stripe)}}). Immediately afterwards,
\{{RecordReaderImpl.readStripe()}} calls \{{StripePlanner.parseStripe(stripe,
fileIncluded)}}, which internally issues a second, identical
\{{DataReader.readStripeFooter(stripe)}} call for the same stripe.
This means every stripe during a scan performs a duplicate footer read (and
protobuf re-parse) from the underlying \{{DataReader}}. For remote/object
stores or high-stripe-count files, this is a measurable, avoidable source of
extra I/O and CPU, and it scales linearly with the number of stripes read.
h3. Root cause
{\{StripePlanner.parseStripe(StripeInformation, boolean[])}} has a single entry
point that unconditionally reads the footer itself, and \{{RecordReaderImpl}}
does not pass along the footer it already obtained in \{{beginReadStripe()}}.
h3. Proposed fix
* Add an overloaded \{{StripePlanner.parseStripe(StripeInformation, boolean[],
OrcProto.StripeFooter)}} that accepts a footer already read by the caller. The
existing two-argument \{{parseStripe}} remains as a thin wrapper that reads the
footer itself, preserving compatibility for other callers.
* In \{{RecordReaderImpl.readStripe()}}, pass the footer already obtained in
\{{beginReadStripe()}} to the new overload, eliminating one
\{{readStripeFooter}} call per stripe.
h3. Scope / compatibility
* No change to the ORC file format or wire behavior.
* Public API is preserved (old \{{parseStripe}} signature retained); no
breaking changes.
* No C++ reader changes needed, this duplication is specific to the Java reader
implementation.
h3. Testing
* New regression test
\{{TestRecordReaderImpl.testNoDuplicateStripeFooterRead}}: uses a Mockito spy
on \{{DataReader}} (with \{{clone()}} stubbed to return the spy, since
\{{RecordReaderImpl}} clones the supplied reader) to assert exactly one
\{{readStripeFooter}} call per stripe, for both a plain scan and an
SArg-filtered scan.
* Lowered read-percentage thresholds in \{{TestMinSeekSize}},
\{{TestRowFilteringComplexTypesNulls}}, and \{{TestRowFilteringIOSkip}} to
reflect the genuinely reduced bytes read per stripe.
was:
## Description
### Problem
When scanning an ORC file, `RecordReaderImpl.beginReadStripe()` already reads
the `OrcProto.StripeFooter` for the current stripe (via
`readStripeFooter(stripe)`). Immediately afterwards,
`RecordReaderImpl.readStripe()` calls `StripePlanner.parseStripe(stripe,
fileIncluded)`, which internally issues a **second, identical**
`DataReader.readStripeFooter(stripe)` call for the same stripe.
This means every stripe during a scan performs a duplicate footer read (and
protobuf re-parse) from the underlying `DataReader`. For remote/object stores
or high-stripe-count files, this is a measurable, avoidable source of extra I/O
and CPU — and it scales linearly with the number of stripes read.
### Root cause
`StripePlanner.parseStripe(StripeInformation, boolean[])` has a single entry
point that unconditionally reads the footer itself, and `RecordReaderImpl` does
not pass along the footer it already obtained in `beginReadStripe()`.
### Proposed fix
- Add an overloaded `StripePlanner.parseStripe(StripeInformation, boolean[],
OrcProto.StripeFooter)` that accepts a footer already read by the caller. The
existing two-argument `parseStripe` remains as a thin wrapper that reads the
footer itself, preserving compatibility for other callers.
- In `RecordReaderImpl.readStripe()`, pass the footer already obtained in
`beginReadStripe()` to the new overload, eliminating one `readStripeFooter`
call per stripe.
### Scope / compatibility
- No change to the ORC file format or wire behavior.
- Public API is preserved (old `parseStripe` signature retained); no breaking
changes.
- No C++ reader changes needed — this duplication is specific to the Java
reader implementation.
### Testing
- New regression test `TestRecordReaderImpl.testNoDuplicateStripeFooterRead`:
uses a Mockito spy on `DataReader` (with `clone()` stubbed to return the spy,
since `RecordReaderImpl` clones the supplied reader) to assert **exactly one**
`readStripeFooter` call per stripe, for both a plain scan and an SArg-filtered
scan.
- Lowered read-percentage thresholds in `TestMinSeekSize`,
`TestRowFilteringComplexTypesNulls`, and `TestRowFilteringIOSkip` to reflect
the genuinely reduced bytes read per stripe.
> [Java] RecordReaderImpl reads and parses each StripeFooter twice per stripe
> during scans
> ----------------------------------------------------------------------------------------
>
> Key: ORC-2223
> URL: https://issues.apache.org/jira/browse/ORC-2223
> Project: ORC
> Issue Type: Improvement
> Reporter: yongzhi.shao
> Priority: Major
>
> Description:
> h3. Problem
> When scanning an ORC file, \{{RecordReaderImpl.beginReadStripe()}} already
> reads the \{{OrcProto.StripeFooter}} for the current stripe (via
> \{{readStripeFooter(stripe)}}). Immediately afterwards,
> \{{RecordReaderImpl.readStripe()}} calls \{{StripePlanner.parseStripe(stripe,
> fileIncluded)}}, which internally issues a second, identical
> \{{DataReader.readStripeFooter(stripe)}} call for the same stripe.
> This means every stripe during a scan performs a duplicate footer read (and
> protobuf re-parse) from the underlying \{{DataReader}}. For remote/object
> stores or high-stripe-count files, this is a measurable, avoidable source of
> extra I/O and CPU, and it scales linearly with the number of stripes read.
> h3. Root cause
> {\{StripePlanner.parseStripe(StripeInformation, boolean[])}} has a single
> entry point that unconditionally reads the footer itself, and
> \{{RecordReaderImpl}} does not pass along the footer it already obtained in
> \{{beginReadStripe()}}.
> h3. Proposed fix
> * Add an overloaded \{{StripePlanner.parseStripe(StripeInformation,
> boolean[], OrcProto.StripeFooter)}} that accepts a footer already read by the
> caller. The existing two-argument \{{parseStripe}} remains as a thin wrapper
> that reads the footer itself, preserving compatibility for other callers.
> * In \{{RecordReaderImpl.readStripe()}}, pass the footer already obtained in
> \{{beginReadStripe()}} to the new overload, eliminating one
> \{{readStripeFooter}} call per stripe.
> h3. Scope / compatibility
> * No change to the ORC file format or wire behavior.
> * Public API is preserved (old \{{parseStripe}} signature retained); no
> breaking changes.
> * No C++ reader changes needed, this duplication is specific to the Java
> reader implementation.
> h3. Testing
> * New regression test
> \{{TestRecordReaderImpl.testNoDuplicateStripeFooterRead}}: uses a Mockito spy
> on \{{DataReader}} (with \{{clone()}} stubbed to return the spy, since
> \{{RecordReaderImpl}} clones the supplied reader) to assert exactly one
> \{{readStripeFooter}} call per stripe, for both a plain scan and an
> SArg-filtered scan.
> * Lowered read-percentage thresholds in \{{TestMinSeekSize}},
> \{{TestRowFilteringComplexTypesNulls}}, and \{{TestRowFilteringIOSkip}} to
> reflect the genuinely reduced bytes read per stripe.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
