Repository navigation
ORC-2223 : [Java] RecordReaderImpl reads and parses each StripeFooter twice per stripe during scans - #2717
Open
BsoBird wants to merge 1 commit into
Open
ORC-2223 : [Java] RecordReaderImpl reads and parses each StripeFooter twice per stripe during scans#2717BsoBird wants to merge 1 commit into
BsoBird wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open findings
parseStripe(..., OrcProto.StripeFooter footer)overload doesn’t appear to perform any… · NewYES_NOfor every row group, but `x <… · Newclone()to return the same instance breaks the typical clone contract (independent copy)… · Newany()matcher here can be less type-safe and may trigger warnings depending on… · New
What changed in this PR
Eliminates redundant StripeFooter reads during Java ORC scans by reusing the already-read footer when planning stripe reads, and updates tests to reflect reduced I/O.
Changes:
- Added an overloaded
StripePlanner.parseStripethat accepts a pre-readOrcProto.StripeFooter. - Updated
RecordReaderImpl.readStripe()to pass the reused footer, avoiding a secondreadStripeFooterper stripe. - Updated/added tests: lowered read-percentage thresholds and introduced a regression test to assert exactly one footer read per stripe.
| File | Description |
|---|---|
| java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java | Adds overload to parse stripes with a caller-provided footer to avoid re-reading. |
| java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java | Uses the reused footer when parsing stripes to eliminate duplicate footer reads. |
| java/core/src/test/org/apache/orc/impl/TestRecordReaderImpl.java | Adds regression test verifying readStripeFooter is called once per stripe. |
| java/core/src/test/org/apache/orc/TestRowFilteringIOSkip.java | Lowers I/O thresholds to match reduced reads after footer dedup. |
| java/core/src/test/org/apache/orc/TestRowFilteringComplexTypesNulls.java | Lowers I/O thresholds to match reduced reads after footer dedup. |
| java/core/src/test/org/apache/orc/TestMinSeekSize.java | Lowers I/O threshold to match reduced reads after footer dedup. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| */ | ||
| public StripePlanner parseStripe(StripeInformation stripe, | ||
| boolean[] columnInclude, | ||
| OrcProto.StripeFooter footer) throws IOException { |
Author
There was a problem hiding this comment.
Thanks for the suggestion — I think we should keep the new three-arg overload declared with throws IOException.
Although the 3-arg overload doesn't itself call DataReader.read* today, it performs parsing work that can surface I/O-related failures (and may in future). Keeping the checked IOException:
- preserves the existing error-handling contract and avoids surprising callers;
- keeps the two-arg wrapper and callers simple (no extra catch/wrap logic required);
- future-proofs the API in case the implementation later needs I/O or underlying helpers throw IOException.
If we want to remove the checked exception, we should do that deliberately and consistently (for example by converting IOExceptions to UncheckedIOException in one place and updating call sites and documentation). I don't recommend silently narrowing the overload's declared exceptions in this patch.
Comment on lines
+2877
to
+2882
| // SArg scan: a predicate that evaluates to YES_NO for every row group so | ||
| // pickRowGroups is exercised but no stripe is skipped. | ||
| SearchArgument sarg = SearchArgumentFactory.newBuilder() | ||
| .startAnd() | ||
| .lessThan("x", PredicateLeaf.Type.LONG, (long) (stripeCount * rowsPerStripe + 1)) | ||
| .end().build(); |
Comment on lines
+2901
to
+2905
| DataReader spied = spy(RecordReaderUtils.createDefaultDataReader(properties)); | ||
| // RecordReaderImpl clones the supplied DataReader before use, so make the | ||
| // spy observe all readStripeFooter calls by returning itself from clone(). | ||
| when(spied.clone()).thenReturn(spied); | ||
| Reader.Options readerOptions = reader.options().dataReader(spied); |
| // drain | ||
| } | ||
| } | ||
| verify(spied, times(stripeCount)).readStripeFooter(any()); |
Copilot stopped reviewing on behalf of
cxzl25 due to an error
October 13, 2026 14:44
BsoBird
force-pushed
the
Fix-StripeFooter-read/parsed-twice-per-stripe
branch
from
October 18, 2026 11:23
9ff6fb5 to
b266311
Compare
…e read - Add an overloaded StripePlanner.parseStripe that accepts an OrcProto.StripeFooter already read by the caller, keeping the existing two-arg parseStripe as a thin wrapper that reads the footer itself (java/core/src/java/org/apache/orc/impl/reader/ StripePlanner.java). - RecordReaderImpl.readStripe now passes the footer obtained in beginReadStripe to planner.parseStripe, eliminating one readStripeFooter call per stripe during scans (java/core/src/java/org/apache/orc/impl/RecordReaderImpl.java). - Lower read-percentage thresholds in TestMinSeekSize, TestRowFilteringComplexTypesNulls, and TestRowFilteringIOSkip to reflect the reduced bytes read per stripe. - Add TestRecordReaderImpl.testNoDuplicateStripeFooterRead, a regression test asserting exactly one footer read per stripe for both plain and SArg-filtered scans.
BsoBird
force-pushed
the
Fix-StripeFooter-read/parsed-twice-per-stripe
branch
from
October 18, 2026 11:33
b266311 to
08fde06
Compare
Author
|
@cxzl25 Hello sir, can u check again? |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
(java): reuse stripe footer in RecordReaderImpl to avoid duplicate read
What changes were proposed in this pull request?
This PR eliminates a duplicate
StripeFooterread in the Java reader: each stripe's footer was being read and parsed twice per scan.RecordReaderImpl.beginReadStripe()already reads theOrcProto.StripeFooterfor the current stripe, butRecordReaderImpl.readStripe()then calledStripePlanner.parseStripe(stripe, fileIncluded), which read the same footer again internally.Changes:
StripePlanner.parseStripe(StripeInformation, boolean[], OrcProto.StripeFooter)that accepts a footer already read by the caller. The existing two-argumentparseStripeis kept as a thin wrapper that reads the footer itself, so other callers are unaffected.RecordReaderImpl.readStripe()now passes the footer obtained inbeginReadStripe()to the new overload, removing oneDataReader.readStripeFootercall per stripe.TestMinSeekSize,TestRowFilteringComplexTypesNulls, andTestRowFilteringIOSkipto reflect the genuinely reduced bytes read per stripe.Why are the changes needed?
Every stripe read during a scan performed a redundant footer read (plus protobuf re-parse) from the underlying
DataReader. On remote/object stores or for files with many stripes, this is a measurable and avoidable source of extra I/O and CPU that scales linearly with the number of stripes read. The fix removes the duplication without changing any read semantics: both reads previously fetched the identical footer bytes for the same stripe.How was this patch tested?
Added a new regression test
TestRecordReaderImpl.testNoDuplicateStripeFooterRead. It installs a Mockito spy on theDataReader(stubbingclone()to return the spy, sinceRecordReaderImplclones the supplied reader) and verifiesreadStripeFooteris invoked exactly once per stripe — covering both a plain scan and an SArg-filtered scan (a predicate that evaluates to YES_NO for every row group, sopickRowGroupsis exercised without skipping whole stripes).Also adjusted the read-percentage assertions in
TestMinSeekSize,TestRowFilteringComplexTypesNulls, andTestRowFilteringIOSkip: those tests assert lower bounds on bytes read, and the fix legitimately reduces bytes read, so thresholds were lowered accordingly (e.g. 5.9 -> 5.0, 0.06 -> 0.03) with comments explaining why.Run: cd java && ./mvnw test -pl core
Was this patch authored or co-authored using generative AI tooling?
Yes. co-authored-by: Claude (Anthropic)