Skip to content

Common: Validate impl class implements base class in DynConstructors #17506

Description

@ArnabKarmakar1108

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

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:

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

  • I can contribute this improvement/feature independently
  • I would be willing to contribute this improvement/feature with guidance from the Iceberg community
  • I cannot contribute this improvement/feature at this time

Metadata

Metadata

Assignees

No one assigned

    Labels

    improvementPR that improves existing functionality

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions