LuciferYang opened a new pull request, #7648:
URL: https://github.com/apache/kyuubi/pull/7648
### Why are the changes needed?
Fix #7647.
`Ctor.newInstanceChecked` truncated arguments to the resolved constructor's
parameter count unconditionally. `Constructor.newInstance` — like
`Method.invoke` — never packs loose varargs arguments (the only valid
reflection form is the packed array as the last argument), so for a **varargs**
constructor the truncation could never produce a correct call:
- when the truncated prefix happened to type-match, trailing arguments were
**silently dropped** and a wrong instance was constructed — e.g. for
`Holder(String, Object...)`, `ctor.newInstance("one", new Object[]{"2a","2b"},
"three")` constructed `Holder("one", ["2a","2b"])` with `"three"` vanishing;
- otherwise the caller got a misleading `argument type mismatch` (an
artifact of the truncated call) instead of the honest `wrong number of
arguments`.
The fix skips the truncation when `ctor.isVarArgs()`: varargs constructors'
arguments pass through unchanged, so callers get either a working packed-array
invocation or the same fail-loudly contract parquet-mr specifies upstream.
Upstream lineage: although the class header says "Copied from
parquet-common", the truncation logic matches Apache Iceberg's copy —
parquet-mr's `newInstanceChecked` has never truncated (its
`TestDynConstructors#testFirstImplReturned` pins extra-arguments-must-throw),
while Iceberg's copy carries the identical unconditional truncation, unfixed
and untested for extra/varargs arguments. This is therefore a Kyuubi-local fix,
recorded in the class javadoc; it may be worth reporting upstream against
Iceberg separately.
Scope: the fixed-arity truncation is long-standing and load-bearing — 7
in-repo sites rely on it for cross-version constructor compatibility
(`TPCDSTable` across Spark versions, `HiveSessionManager` on Hive 2.3,
`HiveTableCatalog`'s `CatalogTable` across SPARK-50675/52729, optional-conf
provider patterns) — so it is deliberately preserved. All in-repo
`DynConstructors` call sites were audited: none passes loose arguments to a
varargs constructor, so no caller's behavior changes except the intended
silent-drop → loud-failure correction.
### How was this patch tested?
Added `DynConstructorsTest`, the first unit tests for `DynConstructors`:
rejection of loose varargs arguments in both forms (packed-plus-extras and
all-loose, asserting the honest `wrong number of arguments` message), the
packed-array happy path through all four entry points (`newInstance`,
`newInstanceChecked`, `invoke`, `invokeChecked`), and the preserved fixed-arity
truncation boundary.
Verified RED→GREEN: with the production change reverted, the rejection test
fails (the old code silently constructs); with the change applied, all pass. A
suite-level revert-and-rerun confirmed the red set matches exactly the claimed
regression pins.
```
build/mvn test -pl kyuubi-util -am # 12/12 pass
build/mvn spotless:check -pl kyuubi-util # pass
```
### Was this patch authored or co-authored using generative AI tooling?
Assisted-by: ZCode with GLM-5.3 (fix authoring, verification, review loop)
The human author reviewed every line of the patch.
--
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]
