chore: Annotate classes/methods/fields that are used by Apache Iceberg #3237

andygrove · 2026-01-22T00:24:21Z

Rationale

Iceberg depends on Comet, therefore Comet has a public API that needs to be maintained. This isn't documented very well.

This PR adds an @IcebergApi annotation to the public API.

~~There is also a new page in the contributor guide documenting this API.~~

I generated this information by analyzing the latest from Iceberg's main branch.

🤖 Generated with Claude Code

Add documentation detailing all Comet classes and methods that form the public API used by Apache Iceberg. This helps contributors understand which APIs may affect Iceberg integration and need backward compatibility considerations. The documentation covers: - org.apache.comet.parquet: FileReader, RowGroupReader, ReadOptions, ParquetColumnSpec, column readers, BatchReader, Native JNI methods - org.apache.comet: CometSchemaImporter - org.apache.comet.vector: CometVector - org.apache.comet.shaded.arrow: RootAllocator, ValueVector Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add a custom Java annotation @IcebergApi to mark all classes, methods, constructors, and fields that form the public API used by Apache Iceberg. This makes it easy to identify which APIs need backward compatibility considerations when making changes. The annotation is applied to: - org.apache.comet.parquet: FileReader, RowGroupReader, ReadOptions, WrappedInputFile, ParquetColumnSpec, AbstractColumnReader, ColumnReader, BatchReader, MetadataColumnReader, ConstantColumnReader, Native, TypeUtil, Utils - org.apache.comet: CometSchemaImporter - org.apache.comet.vector: CometVector Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

Add annotations to: - AbstractColumnReader.nativeHandle (protected field accessed by Iceberg subclasses) - AbstractCometSchemaImporter.close() (called by Iceberg) Also update documentation to include these APIs. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

codecov-commenter · 2026-01-22T01:17:04Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 59.99%. Comparing base (f09f8af) to head (fb2d4d2).
⚠️ Report is 918 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #3237      +/-   ##
============================================
+ Coverage     56.12%   59.99%   +3.86%     
- Complexity      976     1475     +499     
============================================
  Files           119      175      +56     
  Lines         11743    16165    +4422     
  Branches       2251     2681     +430     
============================================
+ Hits           6591     9698    +3107     
- Misses         4012     5114    +1102     
- Partials       1140     1353     +213

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Add a new Maven module containing dedicated unit tests for all @IcebergApi annotated classes, ensuring the public API contract with Apache Iceberg remains stable and tested. Key changes: - Add iceberg-public-api module with 169 tests covering all @IcebergApi classes - Fix CometVector constructor visibility (protected -> public) to match API annotation - Add IcebergApiVerificationTest for reflection-based API verification - Add tests for FileReader, BatchReader, ColumnReader, Native, TypeUtil, Utils - Add tests for CometVector, CometSchemaImporter, WrappedInputFile Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

martin-g · 2026-01-22T20:50:18Z

How this will be maintained in the future ?
Is it expected that the Iceberg maintainers and contributors will open a PR here every time they start using a method/class from Comet that is not yet annotated with @IcebergApi? Or to remove @IcebergApi when they stop using some annotated method ?
Or it will be Comet's devs responsibility to "scan" Iceberg code for new/obsolete usage ?

andygrove · 2026-01-22T21:09:51Z

How this will be maintained in the future ? Is it expected that the Iceberg maintainers and contributors will open a PR here every time they start using a method/class from Comet that is not yet annotated with @IcebergApi? Or to remove @IcebergApi when they stop using some annotated method ? Or it will be Comet's devs responsibility to "scan" Iceberg code for new/obsolete usage ?

The next step is #3240 which adds unit tests to ensure that the API is as expected. This will help prevent accidental changes to the current API methods.

We should probably update the release process documentation to suggest auditing the Iceberg API use before creating a new release.

martin-g · 2026-01-22T21:03:19Z

docs/source/contributor-guide/iceberg_public_api.md

+// Methods used by Iceberg
+public void setRequestedSchemaFromSpecs(List<ParquetColumnSpec> specs)
+public RowGroupReader readNextRowGroup() throws IOException
+public void skipNextRowGroup()


Suggested change

public void skipNextRowGroup()

public boolean skipNextRowGroup()

martin-g · 2026-01-22T21:06:28Z

docs/source/contributor-guide/iceberg_public_api.md

+
+```java
+// Constructor
+public WrappedInputFile(org.apache.iceberg.io.InputFile inputFile)


datafusion-comet/common/src/main/java/org/apache/comet/parquet/WrappedInputFile.java

Line 40 in 640dd03

public WrappedInputFile(Object inputFile) {

actually accepts java.lang.Object

martin-g · 2026-01-22T21:12:57Z

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

+  @After
+  public void tearDown() throws IOException {
+    if (tempDir != null && Files.exists(tempDir)) {
+      Files.walk(tempDir).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);


Suggested change

Files.walk(tempDir).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);

try (Stream<Path> stream : Files.walk(tempDir)) {

stream.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);

}

https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/nio/file/Files.html#walk(java.nio.file.Path,java.nio.file.FileVisitOption...) says:

The returned stream contains references to one or more open directories. The directories are closed by closing the stream.

martin-g · 2026-01-22T21:14:12Z

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

+  /** Checks if native library is available. */
+  protected static boolean isNativeLibraryAvailable() {
+    try {
+      Class.forName("org.apache.comet.NativeBase");


This does not guarantee that the native library is available and loadable.

martin-g · 2026-01-22T21:15:55Z

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

@@ -189,6 +191,7 @@ public BatchReader(
   * @deprecated since 0.10.0, will be removed in 0.11.0.


Maybe it should be undeprecated ?!
Currently is is both deprecated and important (used by Iceberg)

Yes we should remove the deprecated annotation for the time being.

martin-g · 2026-01-22T21:35:14Z

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

+  @Test
+  public void testCurrentBatchMethodExists() throws NoSuchMethodException {
+    Method method = BatchReader.class.getMethod("currentBatch");
+    assertThat(method).isNotNull();


Suggested change

assertThat(method).isNotNull();

martin-g · 2026-01-22T21:35:27Z

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

+  @Test
+  public void testCloseMethodExists() throws NoSuchMethodException {
+    Method method = BatchReader.class.getMethod("close");
+    assertThat(method).isNotNull();


Suggested change

assertThat(method).isNotNull();

martin-g · 2026-01-22T21:38:05Z

docs/source/contributor-guide/iceberg_public_api.md

+// Methods used by Iceberg
+public static AbstractColumnReader getColumnReader(
+    DataType sparkType,
+    ColumnDescriptor descriptor,


The parameter types do not match with https://github.com/apache/datafusion-comet/pull/3237/changes#diff-54de18a6f3ec3c2944f1628012f8c0b0af863da30419a8bc989eb6cd8ccb8cd1R39-R46

Suggested change

ColumnDescriptor descriptor,

ParquetColumnSpec columnSpec,

martin-g · 2026-01-22T21:38:34Z

docs/source/contributor-guide/iceberg_public_api.md

+    CometSchemaImporter importer,
+    int batchSize,
+    boolean useDecimal128,
+    boolean isConstant


Suggested change

boolean isConstant

boolean useLazyMaterialization,

boolean useLegacyTimestamp

martin-g · 2026-01-22T21:42:15Z

docs/source/contributor-guide/iceberg_public_api.md

+
+1. Creates `CometSchemaImporter` with a `RootAllocator`
+2. Uses `Utils.getColumnReader()` to create appropriate column readers
+3. Calls `reset()` and `setPageReader()` for each row group


Which reset() method ?
Maybe:

Suggested change

3. Calls `reset()` and `setPageReader()` for each row group

3. Calls `Native.resetBatch()` and `setPageReader()` for each row group

?!

- 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>

steveloughran · 2026-01-23T17:50:16Z

would it be more resilient to have a @publicapi attribute which took a list of libraries you knew used it it? There's some stuff in hadoop (e.g. hdfs MiniDFSCluster) that document that hbase & others use it...

@InterfaceAudience.LimitedPrivate({"HBase", "HDFS", "Hive", "MapReduce", "Pig"})

andygrove · 2026-02-01T19:34:14Z

would it be more resilient to have a @publicapi attribute which took a list of libraries you knew used it it? There's some stuff in hadoop (e.g. hdfs MiniDFSCluster) that document that hbase & others use it...
@InterfaceAudience.LimitedPrivate({"HBase", "HDFS", "Hive", "MapReduce", "Pig"})

Thanks for the suggestion @steveloughran. Honestly, I'm not anticipating any other systems to have dependencies on Comet, so this may be be overkill right now. However, if this is an approach that people are more comfortable with, then I'm fine with moving to something like this.

@mbutrovich @comphead @parthchandra any thoughts on this?

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

andygrove · 2026-02-01T19:50:46Z

@huaxingao @parthchandra @martin-g in an effort to make this PR easier to review, I changed the scope to just adding the annotations, rather than also adding tests and contributor guide documentation. I would appreciate another review.

parthchandra · 2026-02-02T17:44:49Z

would it be more resilient to have a @publicapi attribute which took a list of libraries you knew used it it? There's some stuff in hadoop (e.g. hdfs MiniDFSCluster) that document that hbase & others use it...
@InterfaceAudience.LimitedPrivate({"HBase", "HDFS", "Hive", "MapReduce", "Pig"})
Thanks for the suggestion @steveloughran. Honestly, I'm not anticipating any other systems to have dependencies on Comet, so this may be be overkill right now. However, if this is an approach that people are more comfortable with, then I'm fine with moving to something like this.

I like this suggestion because it forces us to think in terms of APIs that are for general purpose use. We can do this in a follow up PR, I feel.

parthchandra

Minor comment, otherwise lgtm

parthchandra · 2026-02-02T18:03:41Z

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

@@ -189,6 +191,7 @@ public BatchReader(
   * @deprecated since 0.10.0, will be removed in 0.11.0.


Yes we should remove the deprecated annotation for the time being.

andygrove and others added 2 commits January 21, 2026 17:24

andygrove requested review from huaxingao and parthchandra January 22, 2026 00:34

andygrove and others added 2 commits January 21, 2026 17:35

format

7b58965

huaxingao reviewed Jan 22, 2026

View reviewed changes

common/src/main/java/org/apache/comet/parquet/AbstractColumnReader.java Show resolved Hide resolved

This was referenced Jan 22, 2026

test: Add Iceberg API stability tests #3240

Closed

Move Iceberg-only API classes to iceberg-api module #3241

Closed

Future of Iceberg Support in Comet #2921

Open

andygrove added this to the 0.13.0 milestone Jan 22, 2026

trigger CI

640dd03

martin-g reviewed Jan 22, 2026

View reviewed changes

andygrove removed this from the 0.13.0 milestone Jan 23, 2026

andygrove and others added 2 commits February 1, 2026 12:37

Merge remote-tracking branch 'apache/main' into docs-iceberg-public-api

7c072ec

Remove iceberg-public-api module

ce6cdc1

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>

andygrove changed the title ~~docs: add Iceberg public API documentation to contributor guide~~ chore: Annotate classes/methods/fields that are used by Apache Iceberg Feb 1, 2026

parthchandra approved these changes Feb 2, 2026

View reviewed changes

remove @deprecated annotations from @IcebergApi methods

fb2d4d2

andygrove merged commit 8bd1f24 into apache:main Feb 3, 2026
123 checks passed

andygrove deleted the docs-iceberg-public-api branch February 3, 2026 15:54

	public void skipNextRowGroup()
	public boolean skipNextRowGroup()

-      Files.walk(tempDir).sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+      try (Stream<Path> stream : Files.walk(tempDir)) {
+        stream.sorted(Comparator.reverseOrder()).map(Path::toFile).forEach(File::delete);
+      }

		@@ -189,6 +191,7 @@ public BatchReader(
		* @deprecated since 0.10.0, will be removed in 0.11.0.

	boolean isConstant
	boolean useLazyMaterialization,
	boolean useLegacyTimestamp

	3. Calls `reset()` and `setPageReader()` for each row group
	3. Calls `Native.resetBatch()` and `setPageReader()` for each row group

chore: Annotate classes/methods/fields that are used by Apache Iceberg #3237

chore: Annotate classes/methods/fields that are used by Apache Iceberg #3237

Uh oh!

Conversation

andygrove commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale

Uh oh!

codecov-commenter commented Jan 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

martin-g commented Jan 22, 2026

Uh oh!

andygrove commented Jan 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

steveloughran commented Jan 23, 2026

Uh oh!

andygrove commented Feb 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andygrove commented Feb 1, 2026

Uh oh!

parthchandra commented Feb 2, 2026

Uh oh!

parthchandra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andygrove commented Jan 22, 2026 •

edited

Loading

codecov-commenter commented Jan 22, 2026 •

edited

Loading

andygrove commented Feb 1, 2026 •

edited

Loading