Copilot commented on code in PR #2717:
URL: https://github.com/apache/orc/pull/2717#discussion_r4257692650
##########
java/core/src/test/org/apache/orc/impl/TestRecordReaderImpl.java:
##########
@@ -2817,4 +2822,96 @@ public void testDecimalIsRepeatingFlag() throws
IOException {
}
}
}
+
+ /**
+ * Regression test for the duplicate stripe-footer read. When scanning the
+ * stripes of an ORC file, {@link RecordReaderImpl#readStripe()} reads the
+ * footer via {@link RecordReaderImpl#readStripeFooter(StripeInformation)} in
+ * {@code beginReadStripe} and must reuse that already-read footer when it
+ * calls {@link StripePlanner#parseStripe} instead of reading it a second
+ * time. This asserts exactly one {@code readStripeFooter} call per stripe
+ * for both a plain scan and a SArg-filtered scan.
+ */
+ @Test
+ public void testNoDuplicateStripeFooterRead() throws Exception {
+ Path path = new Path(workDir, "noDupFooterRead.orc");
+ FileSystem fs = FileSystem.get(conf);
+ fs.delete(path, true);
+
+ TypeDescription schema = TypeDescription.fromString("struct<x:int>");
+ // rowIndexStride of 10_000 produces one row group per stripe so that SArg
+ // evaluation still keeps every stripe rather than skipping whole stripes.
+ int rowIndexStride = 10_000;
+ int rowsPerStripe = 10_000;
+ int stripeCount = 3;
+ conf.setLong(OrcConf.STRIPE_ROW_COUNT.getAttribute(), rowsPerStripe);
+ OrcFile.WriterOptions writerOptions =
+ OrcFile.writerOptions(conf).setSchema(schema)
+ .rowIndexStride(rowIndexStride);
+ Writer writer = OrcFile.createWriter(path, writerOptions);
+ VectorizedRowBatch writeBatch = schema.createRowBatch();
+ LongColumnVector writeX = (LongColumnVector) writeBatch.cols[0];
+ int base = 0;
+ int remaining = stripeCount * rowsPerStripe;
+ while (remaining > 0) {
+ writeBatch.reset();
+ int chunk = Math.min(writeBatch.getMaxSize(), remaining);
+ for (int r = 0; r < chunk; ++r) {
+ int idx = writeBatch.size++;
+ writeX.vector[idx] = ++base;
+ }
+ writer.addRowBatch(writeBatch);
+ remaining -= chunk;
+ }
+ writer.close();
+ // Reset the conf so other tests are not affected.
+ conf.set(OrcConf.STRIPE_ROW_COUNT.getAttribute(),
+ String.valueOf(OrcConf.STRIPE_ROW_COUNT.getDefaultValue()));
+
+ Reader reader = OrcFile.createReader(path,
OrcFile.readerOptions(conf).filesystem(fs));
+ assertEquals(stripeCount, reader.getStripes().size());
+
+ // Plain scan: no SArg.
+ runStripeFooterCountTest(reader, stripeCount, null);
+
+ // 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();
Review Comment:
The comment claims the predicate evaluates to `YES_NO` for every row group,
but `x < (stripeCount*rowsPerStripe + 1)` should typically be satisfiable for
all rows and thus evaluate to `YES` based on min/max stats. Either adjust the
comment to reflect the intended behavior (e.g., ‘matches all rows’) or change
the predicate/data generation so `pickRowGroups` reliably sees `YES_NO`
decisions without skipping whole stripes.
##########
java/core/src/test/org/apache/orc/impl/TestRecordReaderImpl.java:
##########
@@ -2817,4 +2822,96 @@ public void testDecimalIsRepeatingFlag() throws
IOException {
}
}
}
+
+ /**
+ * Regression test for the duplicate stripe-footer read. When scanning the
+ * stripes of an ORC file, {@link RecordReaderImpl#readStripe()} reads the
+ * footer via {@link RecordReaderImpl#readStripeFooter(StripeInformation)} in
+ * {@code beginReadStripe} and must reuse that already-read footer when it
+ * calls {@link StripePlanner#parseStripe} instead of reading it a second
+ * time. This asserts exactly one {@code readStripeFooter} call per stripe
+ * for both a plain scan and a SArg-filtered scan.
+ */
+ @Test
+ public void testNoDuplicateStripeFooterRead() throws Exception {
+ Path path = new Path(workDir, "noDupFooterRead.orc");
+ FileSystem fs = FileSystem.get(conf);
+ fs.delete(path, true);
+
+ TypeDescription schema = TypeDescription.fromString("struct<x:int>");
+ // rowIndexStride of 10_000 produces one row group per stripe so that SArg
+ // evaluation still keeps every stripe rather than skipping whole stripes.
+ int rowIndexStride = 10_000;
+ int rowsPerStripe = 10_000;
+ int stripeCount = 3;
+ conf.setLong(OrcConf.STRIPE_ROW_COUNT.getAttribute(), rowsPerStripe);
+ OrcFile.WriterOptions writerOptions =
+ OrcFile.writerOptions(conf).setSchema(schema)
+ .rowIndexStride(rowIndexStride);
+ Writer writer = OrcFile.createWriter(path, writerOptions);
+ VectorizedRowBatch writeBatch = schema.createRowBatch();
+ LongColumnVector writeX = (LongColumnVector) writeBatch.cols[0];
+ int base = 0;
+ int remaining = stripeCount * rowsPerStripe;
+ while (remaining > 0) {
+ writeBatch.reset();
+ int chunk = Math.min(writeBatch.getMaxSize(), remaining);
+ for (int r = 0; r < chunk; ++r) {
+ int idx = writeBatch.size++;
+ writeX.vector[idx] = ++base;
+ }
+ writer.addRowBatch(writeBatch);
+ remaining -= chunk;
+ }
+ writer.close();
+ // Reset the conf so other tests are not affected.
+ conf.set(OrcConf.STRIPE_ROW_COUNT.getAttribute(),
+ String.valueOf(OrcConf.STRIPE_ROW_COUNT.getDefaultValue()));
+
+ Reader reader = OrcFile.createReader(path,
OrcFile.readerOptions(conf).filesystem(fs));
+ assertEquals(stripeCount, reader.getStripes().size());
+
+ // Plain scan: no SArg.
+ runStripeFooterCountTest(reader, stripeCount, null);
+
+ // 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();
+ Reader sargReader = OrcFile.createReader(path,
OrcFile.readerOptions(conf).filesystem(fs));
+ runStripeFooterCountTest(sargReader, stripeCount, sarg);
+ }
+
+ private void runStripeFooterCountTest(Reader reader, int stripeCount,
+ SearchArgument sarg) throws
IOException {
+ ReaderImpl impl = (ReaderImpl) reader;
+ InStream.StreamOptions unencryptedOptions =
+ InStream.options()
+ .withCodec(OrcCodecPool.getCodec(reader.getCompressionKind()))
+ .withBufferSize(reader.getCompressionSize());
+ DataReaderProperties properties =
+ DataReaderProperties.builder()
+ .withCompression(unencryptedOptions)
+ .withFileSystemSupplier(impl.getFileSystemSupplier())
+ .withPath(impl.path)
+ .withZeroCopy(false)
+ .build();
+ 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);
Review Comment:
Stubbing `clone()` to return the same instance breaks the typical clone
contract (independent copy) and can introduce shared mutable state or
close/lifecycle issues in the test (especially if the implementation relies on
per-reader state). A more robust approach is to use a small delegating
`DataReader` wrapper that increments a shared counter on `readStripeFooter`,
and whose `clone()` returns another wrapper sharing that same counter while
delegating to a real cloned reader.
##########
java/core/src/test/org/apache/orc/impl/TestRecordReaderImpl.java:
##########
@@ -2817,4 +2822,96 @@ public void testDecimalIsRepeatingFlag() throws
IOException {
}
}
}
+
+ /**
+ * Regression test for the duplicate stripe-footer read. When scanning the
+ * stripes of an ORC file, {@link RecordReaderImpl#readStripe()} reads the
+ * footer via {@link RecordReaderImpl#readStripeFooter(StripeInformation)} in
+ * {@code beginReadStripe} and must reuse that already-read footer when it
+ * calls {@link StripePlanner#parseStripe} instead of reading it a second
+ * time. This asserts exactly one {@code readStripeFooter} call per stripe
+ * for both a plain scan and a SArg-filtered scan.
+ */
+ @Test
+ public void testNoDuplicateStripeFooterRead() throws Exception {
+ Path path = new Path(workDir, "noDupFooterRead.orc");
+ FileSystem fs = FileSystem.get(conf);
+ fs.delete(path, true);
+
+ TypeDescription schema = TypeDescription.fromString("struct<x:int>");
+ // rowIndexStride of 10_000 produces one row group per stripe so that SArg
+ // evaluation still keeps every stripe rather than skipping whole stripes.
+ int rowIndexStride = 10_000;
+ int rowsPerStripe = 10_000;
+ int stripeCount = 3;
+ conf.setLong(OrcConf.STRIPE_ROW_COUNT.getAttribute(), rowsPerStripe);
+ OrcFile.WriterOptions writerOptions =
+ OrcFile.writerOptions(conf).setSchema(schema)
+ .rowIndexStride(rowIndexStride);
+ Writer writer = OrcFile.createWriter(path, writerOptions);
+ VectorizedRowBatch writeBatch = schema.createRowBatch();
+ LongColumnVector writeX = (LongColumnVector) writeBatch.cols[0];
+ int base = 0;
+ int remaining = stripeCount * rowsPerStripe;
+ while (remaining > 0) {
+ writeBatch.reset();
+ int chunk = Math.min(writeBatch.getMaxSize(), remaining);
+ for (int r = 0; r < chunk; ++r) {
+ int idx = writeBatch.size++;
+ writeX.vector[idx] = ++base;
+ }
+ writer.addRowBatch(writeBatch);
+ remaining -= chunk;
+ }
+ writer.close();
+ // Reset the conf so other tests are not affected.
+ conf.set(OrcConf.STRIPE_ROW_COUNT.getAttribute(),
+ String.valueOf(OrcConf.STRIPE_ROW_COUNT.getDefaultValue()));
+
+ Reader reader = OrcFile.createReader(path,
OrcFile.readerOptions(conf).filesystem(fs));
+ assertEquals(stripeCount, reader.getStripes().size());
+
+ // Plain scan: no SArg.
+ runStripeFooterCountTest(reader, stripeCount, null);
+
+ // 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();
+ Reader sargReader = OrcFile.createReader(path,
OrcFile.readerOptions(conf).filesystem(fs));
+ runStripeFooterCountTest(sargReader, stripeCount, sarg);
+ }
+
+ private void runStripeFooterCountTest(Reader reader, int stripeCount,
+ SearchArgument sarg) throws
IOException {
+ ReaderImpl impl = (ReaderImpl) reader;
+ InStream.StreamOptions unencryptedOptions =
+ InStream.options()
+ .withCodec(OrcCodecPool.getCodec(reader.getCompressionKind()))
+ .withBufferSize(reader.getCompressionSize());
+ DataReaderProperties properties =
+ DataReaderProperties.builder()
+ .withCompression(unencryptedOptions)
+ .withFileSystemSupplier(impl.getFileSystemSupplier())
+ .withPath(impl.path)
+ .withZeroCopy(false)
+ .build();
+ 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);
+ if (sarg != null) {
+ readerOptions.searchArgument(sarg, new String[] {"x"});
+ }
+ VectorizedRowBatch batch = reader.getSchema().createRowBatch();
+ try (RecordReader rows = reader.rows(readerOptions)) {
+ while (rows.nextBatch(batch)) {
+ // drain
+ }
+ }
+ verify(spied, times(stripeCount)).readStripeFooter(any());
Review Comment:
Using a raw `any()` matcher here can be less type-safe and may trigger
warnings depending on Mockito configuration. Prefer a typed matcher (e.g.,
`any(StripeInformation.class)`) to ensure the verification matches the intended
overload/signature.
##########
java/core/src/java/org/apache/orc/impl/reader/StripePlanner.java:
##########
@@ -140,7 +140,22 @@ public StripePlanner(StripePlanner old) {
*/
public StripePlanner parseStripe(StripeInformation stripe,
boolean[] columnInclude) throws IOException
{
- OrcProto.StripeFooter footer = dataReader.readStripeFooter(stripe);
+ return parseStripe(stripe, columnInclude,
dataReader.readStripeFooter(stripe));
+ }
+
+ /**
+ * Parse a new stripe using a footer that was already read. Resets the
+ * current stripe state. This avoids a redundant {@code readStripeFooter}
+ * call when the caller (e.g. {@code RecordReaderImpl}) has already read
+ * the footer for the same stripe.
+ * @param stripe the new stripe
+ * @param columnInclude an array with true for each column to read
+ * @param footer the stripe footer that was already read for {@code stripe}
+ * @return this for method chaining
+ */
+ public StripePlanner parseStripe(StripeInformation stripe,
+ boolean[] columnInclude,
+ OrcProto.StripeFooter footer) throws
IOException {
Review Comment:
The new `parseStripe(..., OrcProto.StripeFooter footer)` overload doesn’t
appear to perform any I/O, but it declares `throws IOException`. That
unnecessarily broadens the API contract for callers and makes usage noisier.
Consider removing `throws IOException` from the 3-arg overload (keeping it on
the 2-arg overload that actually reads the footer).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
