-
Notifications
You must be signed in to change notification settings - Fork 272
Description
Summary
Move classes that are exclusively used by Iceberg (not used by any non-@IcebergApi Comet code) from common into the new iceberg-api module, added in #3240. This will improve the separation of concerns and make the Iceberg API boundary clearer.
Classes to Move
The following 4 classes are only referenced by other @IcebergApi classes and exist purely for Iceberg integration:
| Class | Current Location | Purpose |
|---|---|---|
ParquetColumnSpec |
common/src/main/java/org/apache/comet/parquet/ |
Bridge class to pass column descriptors between Comet and Iceberg (since Iceberg shades Parquet) |
RowGroupReader |
common/src/main/java/org/apache/comet/parquet/ |
PageReadStore implementation for row group reading |
WrappedInputFile |
common/src/main/java/org/apache/comet/parquet/ |
Wraps Iceberg's InputFile to implement Parquet's InputFile interface |
AbstractCometSchemaImporter |
common/src/main/java/org/apache/arrow/c/ |
Base class for schema import functionality |
Important Considerations
Packaging/Shading Concerns
-
Iceberg shades Parquet -
ParquetColumnSpecexists specifically because Iceberg uses shaded Parquet classes. Any module structure changes must preserve this compatibility. -
JAR structure - Ensure the classes remain accessible to Iceberg after the move. The final packaged JAR must include these classes in a location Iceberg can access.
-
Maven shade plugin configuration - Review and update shade plugin configuration if needed to ensure:
- Classes are included in the shaded JAR
- Package relocations don't break Iceberg's access
- No duplicate classes across modules
-
Dependency direction - The
iceberg-public-apimodule currently only contains tests. If we move source classes there, we need to:- Update module dependencies
- Ensure
commonmodule can still depend on these classes if needed - Consider if a separate
iceberg-apimodule (non-test) is more appropriate
Testing Requirements
- Verify Iceberg integration tests pass after the move
- Verify shaded JAR contains all necessary classes
- Test with actual Iceberg to ensure no classloader issues
- Run
iceberg-public-apimodule tests
Alternative Approaches
- Keep in
common, just document - Leave classes incommonbut document they're Iceberg-specific - New
iceberg-apisource module - Create a separate source module (not test-only) for Iceberg API classes - Subpackage organization - Move to
org.apache.comet.iceberg.*package withincommon
Related
- PR test: Add Iceberg API stability tests #3240 - Added
iceberg-public-apitest module - PR docs: add Iceberg public API documentation to contributor guide #3237 - Added
@IcebergApiannotation