ArnabKarmakar1108 opened a new issue, #17506:
URL: https://github.com/apache/iceberg/issues/17506
### Feature Request / Improvement
## Problem
`DynConstructors.Builder` accepts a `baseClass` via
`DynConstructors.builder(Class<?> baseClass)` and is used throughout Iceberg to
load pluggable implementations at runtime (e.g. `Catalog`, `FileIO`,
`AuthManager`, `LockManager`).
When an implementation class does **not** implement or extend the expected
base class, the error is deferred until `Ctor.newInstance()` is called, where
it surfaces as a `ClassCastException`. This makes misconfiguration (wrong class
name in catalog properties, typos, etc.) harder to diagnose.
There are existing TODOs in `TestDynConstructors` documenting this gap:
-
`common/src/test/java/org/apache/iceberg/common/TestDynConstructors.java:47` —
string-based `.impl(...)` with an unrelated class
-
`common/src/test/java/org/apache/iceberg/common/TestDynConstructors.java:60` —
class-based `.impl(...)` with an unrelated class
Current behavior (both tests expect `ClassCastException` at `newInstance()`
time):
```java
DynConstructors.Ctor<MyInterface> ctor =
DynConstructors.builder(MyInterface.class)
.impl(MyUnrelatedClass.class) // does not implement MyInterface
.buildChecked();
ctor.newInstance(); // ClassCastException here
```
## Proposed behavior
When `baseClass` is set on the builder, validate that the resolved
implementation class is assignable to `baseClass` at **build time** (`build()`
/ `buildChecked()`), and fail fast with a clear, actionable error.
For example:
```java
assertThatThrownBy(() ->
DynConstructors.builder(MyInterface.class)
.impl(MyUnrelatedClass.class)
.buildChecked())
.isInstanceOf(IllegalArgumentException.class)
.hasMessageContaining("does not implement");
```
The same validation should apply to both `.impl(String, ...)` and
`.impl(Class, ...)` (and likely `.hiddenImpl(...)` variants as well).
## Scope
- **Module:** `common` (`DynConstructors.java`)
- **Tests:** Update `TestDynConstructors.testInterfaceWrongImplString` and
`TestDynConstructors.testInterfaceWrongImplClass` to assert build-time failure
instead of `ClassCastException` at instantiation
- **Impact:** Improves error messages for all runtime plugin loading paths
that use `DynConstructors.builder(SomeInterface.class)` (Catalog, FileIO,
AuthManager, etc.)
## Notes
- `DynConstructors` is copied from parquet-common; consider whether a
similar upstream fix is warranted, but Iceberg can fix locally regardless.
- When `builder()` is called without a `baseClass`, no assignability check
is needed (existing behavior for
`DynConstructors.builder().impl(MyClass.class)` should remain unchanged).
### Query engine
_No response_
### Willingness to contribute
- [x] I can contribute this improvement/feature independently
- [x] I would be willing to contribute this improvement/feature with
guidance from the Iceberg community
- [ ] I cannot contribute this improvement/feature at this time
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
