Copilot commented on code in PR #2722:
URL: https://github.com/apache/orc/pull/2722#discussion_r4345764222
##########
java/core/src/test/org/apache/orc/impl/TestInStream.java:
##########
@@ -1000,4 +1000,73 @@ public void testStreamResetWithoutIncreasedLength()
throws IOException {
byte[] inBuffer = new byte[5];
assertEquals(5, inStream.read(inBuffer));
}
+
+ @Test
+ public void testUncompressedSeekToEndOfStream() throws Exception {
+ // ORC-2226: the row group index of an all-null trailing row group points
+ // at the end of the stream, and the read planner may not have read any
+ // bytes for the stream, so seeking to the end must be accepted.
+ InStream in = InStream.create("test", null, 0, 1024);
+ InStream.UncompressedStream stream = (InStream.UncompressedStream) in;
+ stream.seek(0);
+ stream.seek(1024);
+ assertEquals(-1, in.read());
+ assertEquals(0, in.available());
+ try {
+ stream.seek(1025);
+ fail();
+ } catch (IllegalArgumentException e) {
+ // expected: past the end of the stream
+ }
+ }
+
+ @Test
+ public void testCompressedSeekToEndOfStream() throws Exception {
+ // ORC-2226: same as above, for compressed streams.
+ InStream in = InStream.create("test", null, 0, 1024,
+ InStream.options().withCodec(new ZlibCodec()).withBufferSize(300));
+ InStream.CompressedStream stream = (InStream.CompressedStream) in;
+ stream.seek(0);
+ stream.seek(1024);
+ assertEquals(-1, in.read());
+ assertEquals(0, in.available());
+ try {
+ stream.seek(1025);
+ fail();
+ } catch (IOException e) {
+ // expected: past the end of the stream
+ }
Review Comment:
This test expects an `IOException` when seeking past the end, but the stream
implementations commonly signal out-of-bounds seeks via
`IllegalArgumentException` (as your uncompressed test does). If
`CompressedStream.seek(long)` throws `IllegalArgumentException`, this test will
fail. Prefer `assertThrows(IllegalArgumentException.class, () ->
stream.seek(1025))` (JUnit 4.13+) or catch `IllegalArgumentException` here to
align with the actual contract.
##########
java/core/src/test/org/apache/orc/impl/TestRecordReaderImpl.java:
##########
@@ -2817,4 +2817,121 @@ public void testDecimalIsRepeatingFlag() throws
IOException {
}
}
}
+
+ @Test
+ public void testSargOnAllNullTrailingRowGroup() throws Exception {
+ // ORC-2226: with predicate pushdown, selecting only an all-null trailing
+ // row group of an uncompressed stripe makes the read planner assign zero
+ // bytes to the column's DATA stream; seeking it to the recorded position
+ // (the end of the stream) must not fail and must read all-null values.
+ Path path = new Path(workDir, "allNullTrailingRowGroup.orc");
+ FileSystem.get(conf).delete(path, true);
+ TypeDescription schema =
TypeDescription.fromString("struct<k:string,v:string>");
+ Writer writer = OrcFile.createWriter(path, OrcFile.writerOptions(conf)
+ .setSchema(schema)
+ .compress(CompressionKind.NONE)
+ .rowIndexStride(1024));
+ VectorizedRowBatch batch = schema.createRowBatch();
+ BytesColumnVector k = (BytesColumnVector) batch.cols[0];
+ BytesColumnVector v = (BytesColumnVector) batch.cols[1];
+ batch.size = 1024;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "a".getBytes(StandardCharsets.UTF_8));
+ // distinct values so that the writer uses DIRECT encoding
+ v.setVal(i, String.format("%07dx", i).getBytes(StandardCharsets.UTF_8));
Review Comment:
These tests use `String.format(...)` without specifying a locale, which
makes output depend on the JVM default locale. To keep tests deterministic
across environments, use `String.format(Locale.ROOT, ...)` for these formatted
values.
##########
java/core/src/test/org/apache/orc/impl/TestRecordReaderImpl.java:
##########
@@ -2817,4 +2817,121 @@ public void testDecimalIsRepeatingFlag() throws
IOException {
}
}
}
+
+ @Test
+ public void testSargOnAllNullTrailingRowGroup() throws Exception {
+ // ORC-2226: with predicate pushdown, selecting only an all-null trailing
+ // row group of an uncompressed stripe makes the read planner assign zero
+ // bytes to the column's DATA stream; seeking it to the recorded position
+ // (the end of the stream) must not fail and must read all-null values.
+ Path path = new Path(workDir, "allNullTrailingRowGroup.orc");
+ FileSystem.get(conf).delete(path, true);
+ TypeDescription schema =
TypeDescription.fromString("struct<k:string,v:string>");
+ Writer writer = OrcFile.createWriter(path, OrcFile.writerOptions(conf)
+ .setSchema(schema)
+ .compress(CompressionKind.NONE)
+ .rowIndexStride(1024));
+ VectorizedRowBatch batch = schema.createRowBatch();
+ BytesColumnVector k = (BytesColumnVector) batch.cols[0];
+ BytesColumnVector v = (BytesColumnVector) batch.cols[1];
+ batch.size = 1024;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "a".getBytes(StandardCharsets.UTF_8));
+ // distinct values so that the writer uses DIRECT encoding
+ v.setVal(i, String.format("%07dx", i).getBytes(StandardCharsets.UTF_8));
+ }
+ writer.addRowBatch(batch);
+ batch.reset();
+ batch.size = 512;
+ v.noNulls = false;
+ v.isRepeating = true;
+ v.isNull[0] = true;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "c".getBytes(StandardCharsets.UTF_8));
+ }
+ writer.addRowBatch(batch);
+ writer.close();
+
+ Reader reader = OrcFile.createReader(path, OrcFile.readerOptions(conf));
+ SearchArgument sarg = SearchArgumentFactory.newBuilder()
+ .equals("k", PredicateLeaf.Type.STRING, "c")
+ .build();
+ VectorizedRowBatch readBatch = reader.getSchema().createRowBatch();
+ long rowCount = 0;
+ try (RecordReader rows = reader.rows(reader.options()
+ .searchArgument(sarg, new String[] {"k"}))) {
+ while (rows.nextBatch(readBatch)) {
+ BytesColumnVector readK = (BytesColumnVector) readBatch.cols[0];
+ BytesColumnVector readV = (BytesColumnVector) readBatch.cols[1];
+ for (int i = 0; i < readBatch.size; i++) {
+ assertEquals("c", readK.toString(i));
+ assertTrue(readV.isRepeating ? readV.isNull[0] : readV.isNull[i]);
+ }
+ rowCount += readBatch.size;
+ }
+ }
+ assertEquals(512, rowCount);
+ }
+
+ @Test
+ public void testSargOnAllNullMiddleRowGroup() throws Exception {
+ // An all-null row group in the middle of a stripe is covered by the
+ // following row group's estimated end offset; make sure that such a
+ // selection keeps working.
+ Path path = new Path(workDir, "allNullMiddleRowGroup.orc");
+ FileSystem.get(conf).delete(path, true);
+ TypeDescription schema =
TypeDescription.fromString("struct<k:string,v:string>");
+ Writer writer = OrcFile.createWriter(path, OrcFile.writerOptions(conf)
+ .setSchema(schema)
+ .compress(CompressionKind.NONE)
+ .rowIndexStride(1024));
+ VectorizedRowBatch batch = schema.createRowBatch();
+ BytesColumnVector k = (BytesColumnVector) batch.cols[0];
+ BytesColumnVector v = (BytesColumnVector) batch.cols[1];
+ batch.size = 1024;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "a".getBytes(StandardCharsets.UTF_8));
+ v.setVal(i, String.format("%07dx", i).getBytes(StandardCharsets.UTF_8));
Review Comment:
These tests use `String.format(...)` without specifying a locale, which
makes output depend on the JVM default locale. To keep tests deterministic
across environments, use `String.format(Locale.ROOT, ...)` for these formatted
values.
##########
java/core/src/test/org/apache/orc/impl/TestRecordReaderImpl.java:
##########
@@ -2817,4 +2817,121 @@ public void testDecimalIsRepeatingFlag() throws
IOException {
}
}
}
+
+ @Test
+ public void testSargOnAllNullTrailingRowGroup() throws Exception {
+ // ORC-2226: with predicate pushdown, selecting only an all-null trailing
+ // row group of an uncompressed stripe makes the read planner assign zero
+ // bytes to the column's DATA stream; seeking it to the recorded position
+ // (the end of the stream) must not fail and must read all-null values.
+ Path path = new Path(workDir, "allNullTrailingRowGroup.orc");
+ FileSystem.get(conf).delete(path, true);
+ TypeDescription schema =
TypeDescription.fromString("struct<k:string,v:string>");
+ Writer writer = OrcFile.createWriter(path, OrcFile.writerOptions(conf)
+ .setSchema(schema)
+ .compress(CompressionKind.NONE)
+ .rowIndexStride(1024));
+ VectorizedRowBatch batch = schema.createRowBatch();
+ BytesColumnVector k = (BytesColumnVector) batch.cols[0];
+ BytesColumnVector v = (BytesColumnVector) batch.cols[1];
+ batch.size = 1024;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "a".getBytes(StandardCharsets.UTF_8));
+ // distinct values so that the writer uses DIRECT encoding
+ v.setVal(i, String.format("%07dx", i).getBytes(StandardCharsets.UTF_8));
+ }
+ writer.addRowBatch(batch);
+ batch.reset();
+ batch.size = 512;
+ v.noNulls = false;
+ v.isRepeating = true;
+ v.isNull[0] = true;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "c".getBytes(StandardCharsets.UTF_8));
+ }
+ writer.addRowBatch(batch);
+ writer.close();
+
+ Reader reader = OrcFile.createReader(path, OrcFile.readerOptions(conf));
+ SearchArgument sarg = SearchArgumentFactory.newBuilder()
+ .equals("k", PredicateLeaf.Type.STRING, "c")
+ .build();
+ VectorizedRowBatch readBatch = reader.getSchema().createRowBatch();
+ long rowCount = 0;
+ try (RecordReader rows = reader.rows(reader.options()
+ .searchArgument(sarg, new String[] {"k"}))) {
+ while (rows.nextBatch(readBatch)) {
+ BytesColumnVector readK = (BytesColumnVector) readBatch.cols[0];
+ BytesColumnVector readV = (BytesColumnVector) readBatch.cols[1];
+ for (int i = 0; i < readBatch.size; i++) {
+ assertEquals("c", readK.toString(i));
+ assertTrue(readV.isRepeating ? readV.isNull[0] : readV.isNull[i]);
+ }
+ rowCount += readBatch.size;
+ }
+ }
+ assertEquals(512, rowCount);
+ }
+
+ @Test
+ public void testSargOnAllNullMiddleRowGroup() throws Exception {
+ // An all-null row group in the middle of a stripe is covered by the
+ // following row group's estimated end offset; make sure that such a
+ // selection keeps working.
+ Path path = new Path(workDir, "allNullMiddleRowGroup.orc");
+ FileSystem.get(conf).delete(path, true);
+ TypeDescription schema =
TypeDescription.fromString("struct<k:string,v:string>");
+ Writer writer = OrcFile.createWriter(path, OrcFile.writerOptions(conf)
+ .setSchema(schema)
+ .compress(CompressionKind.NONE)
+ .rowIndexStride(1024));
+ VectorizedRowBatch batch = schema.createRowBatch();
+ BytesColumnVector k = (BytesColumnVector) batch.cols[0];
+ BytesColumnVector v = (BytesColumnVector) batch.cols[1];
+ batch.size = 1024;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "a".getBytes(StandardCharsets.UTF_8));
+ v.setVal(i, String.format("%07dx", i).getBytes(StandardCharsets.UTF_8));
+ }
+ writer.addRowBatch(batch);
+ batch.reset();
+ batch.size = 1024;
+ v.noNulls = false;
+ v.isRepeating = true;
+ v.isNull[0] = true;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "b".getBytes(StandardCharsets.UTF_8));
+ }
+ writer.addRowBatch(batch);
+ batch.reset();
+ batch.size = 1024;
+ v.noNulls = true;
+ v.isRepeating = false;
+ for (int i = 0; i < batch.size; i++) {
+ k.setVal(i, "d".getBytes(StandardCharsets.UTF_8));
+ v.setVal(i, String.format("%07dy", i).getBytes(StandardCharsets.UTF_8));
Review Comment:
These tests use `String.format(...)` without specifying a locale, which
makes output depend on the JVM default locale. To keep tests deterministic
across environments, use `String.format(Locale.ROOT, ...)` for these formatted
values.
--
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]
