Skip to content

Commit a4ca885

Browse files
andygroveclaude
andcommitted
Address review comments
- Fix documentation: skipNextRowGroup returns boolean, WrappedInputFile takes Object, update Utils.getColumnReader signature, fix TypeUtil reference, clarify Native.resetBatch() - Use try-with-resources for Files.walk() in AbstractApiTest - Remove unnecessary isNotNull() assertions in test files (getMethod/getConstructor throw if not found) - Add @IcebergApi to BatchReader.close() and ReadOptions.Builder constructor Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 640dd03 commit a4ca885

6 files changed

Lines changed: 15 additions & 20 deletions

File tree

common/src/main/java/org/apache/comet/parquet/BatchReader.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -530,6 +530,7 @@ public boolean nextBatch(int batchSize) {
530530
return true;
531531
}
532532

533+
@IcebergApi
533534
@Override
534535
public void close() throws IOException {
535536
if (columnReaders != null) {

common/src/main/java/org/apache/comet/parquet/ReadOptions.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,7 @@ public ReadOptions build() {
148148
adjustReadRangeSkew);
149149
}
150150

151+
@IcebergApi
151152
public Builder(Configuration conf) {
152153
this.conf = conf;
153154
this.parallelIOEnabled =

docs/source/contributor-guide/iceberg_public_api.md

Lines changed: 9 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ public FileReader(
6262
// Methods used by Iceberg
6363
public void setRequestedSchemaFromSpecs(List<ParquetColumnSpec> specs)
6464
public RowGroupReader readNextRowGroup() throws IOException
65-
public void skipNextRowGroup()
65+
public boolean skipNextRowGroup()
6666
public void close() throws IOException
6767
```
6868

@@ -94,7 +94,7 @@ Wrapper that adapts Iceberg's `InputFile` interface to Comet's file reading infr
9494

9595
```java
9696
// Constructor
97-
public WrappedInputFile(org.apache.iceberg.io.InputFile inputFile)
97+
public WrappedInputFile(Object inputFile)
9898
```
9999

100100
### ParquetColumnSpec
@@ -223,13 +223,14 @@ General utility methods.
223223

224224
```java
225225
// Methods used by Iceberg
226-
public static AbstractColumnReader getColumnReader(
227-
DataType sparkType,
228-
ColumnDescriptor descriptor,
226+
public static ColumnReader getColumnReader(
227+
DataType type,
228+
ParquetColumnSpec columnSpec,
229229
CometSchemaImporter importer,
230230
int batchSize,
231231
boolean useDecimal128,
232-
boolean isConstant
232+
boolean useLazyMaterialization,
233+
boolean useLegacyTimestamp
233234
)
234235
```
235236

@@ -308,15 +309,15 @@ Arrow's base vector interface (shaded). Used as return type in `CometVector.getV
308309
1. Iceberg creates a `WrappedInputFile` from its `InputFile`
309310
2. Creates `ReadOptions` via builder pattern
310311
3. Instantiates `FileReader` with the wrapped input file
311-
4. Converts Parquet `ColumnDescriptor`s to `ParquetColumnSpec`s using `CometTypeUtils`
312+
4. Converts Parquet `ColumnDescriptor`s to `ParquetColumnSpec`s using `TypeUtil`
312313
5. Calls `setRequestedSchemaFromSpecs()` to specify which columns to read
313314
6. Iterates through row groups via `readNextRowGroup()` and `skipNextRowGroup()`
314315

315316
### Column Reading Flow
316317

317318
1. Creates `CometSchemaImporter` with a `RootAllocator`
318319
2. Uses `Utils.getColumnReader()` to create appropriate column readers
319-
3. Calls `reset()` and `setPageReader()` for each row group
320+
3. Calls `Native.resetBatch()` and `setPageReader()` for each row group
320321
4. Uses `BatchReader` to coordinate reading batches across all columns
321322
5. Retrieves results via `delegate().currentBatch()`
322323

iceberg-public-api/src/test/java/org/apache/comet/iceberg/api/AbstractApiTest.java

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,9 @@ public void setUp() throws IOException {
4949
@After
5050
public void tearDown() throws IOException {
5151
if (tempDir != null && Files.exists(tempDir)) {
52-
Files.walk(tempDir).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
52+
try (var stream = Files.walk(tempDir)) {
53+
stream.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
54+
}
5355
}
5456
}
5557

iceberg-public-api/src/test/java/org/apache/comet/iceberg/api/parquet/AbstractColumnReaderApiTest.java

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -56,36 +56,31 @@ public void testImplementsAutoCloseable() {
5656
@Test
5757
public void testSetBatchSizeMethodExists() throws NoSuchMethodException {
5858
Method method = AbstractColumnReader.class.getMethod("setBatchSize", int.class);
59-
assertThat(method).isNotNull();
6059
assertThat(hasIcebergApiAnnotation(method)).isTrue();
6160
}
6261

6362
@Test
6463
public void testCloseMethodExists() throws NoSuchMethodException {
6564
Method method = AbstractColumnReader.class.getMethod("close");
66-
assertThat(method).isNotNull();
6765
assertThat(hasIcebergApiAnnotation(method)).isTrue();
6866
}
6967

7068
@Test
7169
public void testReadBatchMethodExists() throws NoSuchMethodException {
7270
Method method = AbstractColumnReader.class.getMethod("readBatch", int.class);
73-
assertThat(method).isNotNull();
7471
assertThat(Modifier.isAbstract(method.getModifiers())).isTrue();
7572
}
7673

7774
@Test
7875
public void testCurrentBatchMethodExists() throws NoSuchMethodException {
7976
Method method = AbstractColumnReader.class.getMethod("currentBatch");
80-
assertThat(method).isNotNull();
8177
assertThat(Modifier.isAbstract(method.getModifiers())).isTrue();
8278
assertThat(method.getReturnType().getSimpleName()).isEqualTo("CometVector");
8379
}
8480

8581
@Test
8682
public void testNativeHandleFieldExists() throws NoSuchFieldException {
8783
Field field = AbstractColumnReader.class.getDeclaredField("nativeHandle");
88-
assertThat(field).isNotNull();
8984
assertThat(hasIcebergApiAnnotation(field)).isTrue();
9085
assertThat(Modifier.isProtected(field.getModifiers())).isTrue();
9186
}

iceberg-public-api/src/test/java/org/apache/comet/iceberg/api/parquet/BatchReaderApiTest.java

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,44 +48,39 @@ public void testBatchReaderIsPublic() {
4848
@Test
4949
public void testConstructorWithColumnReadersExists() throws NoSuchMethodException {
5050
Constructor<?> constructor = BatchReader.class.getConstructor(AbstractColumnReader[].class);
51-
assertThat(constructor).isNotNull();
5251
assertThat(hasIcebergApiAnnotation(constructor)).isTrue();
5352
}
5453

5554
@Test
5655
public void testSetSparkSchemaMethodExists() throws NoSuchMethodException {
5756
Method method = BatchReader.class.getMethod("setSparkSchema", StructType.class);
58-
assertThat(method).isNotNull();
5957
assertThat(hasIcebergApiAnnotation(method)).isTrue();
6058
}
6159

6260
@Test
6361
public void testGetColumnReadersMethodExists() throws NoSuchMethodException {
6462
Method method = BatchReader.class.getMethod("getColumnReaders");
65-
assertThat(method).isNotNull();
6663
assertThat(hasIcebergApiAnnotation(method)).isTrue();
6764
assertThat(method.getReturnType()).isEqualTo(AbstractColumnReader[].class);
6865
}
6966

7067
@Test
7168
public void testNextBatchWithSizeMethodExists() throws NoSuchMethodException {
7269
Method method = BatchReader.class.getMethod("nextBatch", int.class);
73-
assertThat(method).isNotNull();
7470
assertThat(hasIcebergApiAnnotation(method)).isTrue();
7571
assertThat(method.getReturnType()).isEqualTo(boolean.class);
7672
}
7773

7874
@Test
7975
public void testCurrentBatchMethodExists() throws NoSuchMethodException {
8076
Method method = BatchReader.class.getMethod("currentBatch");
81-
assertThat(method).isNotNull();
8277
assertThat(method.getReturnType().getSimpleName()).isEqualTo("ColumnarBatch");
8378
}
8479

8580
@Test
8681
public void testCloseMethodExists() throws NoSuchMethodException {
8782
Method method = BatchReader.class.getMethod("close");
88-
assertThat(method).isNotNull();
83+
assertThat(hasIcebergApiAnnotation(method)).isTrue();
8984
}
9085

9186
@Test

0 commit comments

Comments
 (0)