LuciferYang opened a new pull request, #7700: URL: https://github.com/apache/kyuubi/pull/7700
### Why are the changes needed? Closes #7699. `UnboundMethod.invokeChecked` copied the argument array for every fixed-arity call, including the case where `args.length` already equalled the method's parameter count. There the copy hands `Method.invoke` the same element references and, because `Arrays.copyOfRange(T[], int, int)` preserves the source array's runtime component type, the same carrier type; it is then discarded. The varargs branch has always passed the caller's array through, so this extends the same pass-through to the one case where the copy changes nothing. This is one array allocation per call removed, and nothing more. There is no benchmark and the hotness of the path is unmeasured. The hottest in-repo caller is `RowSet.toHiveString`. It runs per column per row inside a UDF, but on the default `-Pspark-3.5` profile it passes 4 arguments to the 3-argument `HiveResult.toHiveString`, so it takes the truncating branch and gains nothing. It reaches the exact-arity branch only on Spark 4.0 and later. The copy stays for the other two cases, and both are load-bearing. It drops extra arguments, which `HiveConnectorUtils.newStorageFormat` and `copyStorageFormat` rely on to call the 6-argument `CatalogStorageFormat.apply` with 7 arguments on Spark below 4.2; a comment above that call site states the dependency. And it null-pads a short argument list, so a caller that passes too few reaches the target with nulls rather than an exception. That is why the guard has to be `args.length == argLength`. The two obvious edits each break one branch: passing through when the list is longer stops the truncation, and copying only when it is longer, which is what `DynConstructors.newInstanceChecked` does, stops the padding. Note the polarity differs between the two classes, since `DynConstructors` guards the copy while this guards the pass-through. The condition carries a comment saying so. Both upstream copies still copy unconditionally: iceberg-common has the bare `if (argLength < 0)`, and so does parquet-common. This is therefore a deliberate Kyuubi-local divergence in an already-forked file. Behaviour is preserved, so a future resync from upstream would lose the fast path and nothing else. ### How was this patch tested? Nothing observable changes at equal arity, so no test can distinguish this patch from its revert. For a fixed-arity method the argument array is only the reflection carrier: `Method.invoke` passes `args[i]` to the callee and never `args` itself, so the carrier's identity cannot reach user code. Any assertion written against the change would pass with it reverted, which AGENTS.md rules out. What the patch does add is coverage for the two branches it deliberately preserves, neither of which had any: - `testInvokeDropsExtraArgumentsForFixedArity` invokes a 2-argument method with 3 arguments and asserts the third is dropped, which is the behaviour `HiveConnectorUtils` depends on. - `testInvokePadsMissingArgumentsWithNull` invokes it with 1 argument and asserts the target sees a trailing null. Both pass before and after this patch, since the old code copied unconditionally and so did both things. They are not vacuous with respect to the guard, though. Changing `==` to `>` makes the first fail with `IllegalArgumentException: wrong number of arguments`; changing it to `<=`, which is the `DynConstructors`-equivalent shape, makes the second fail the same way. Together they pin `==` as the only guard that keeps both branches working. Equivalence itself was checked by running a probe against classes built before and after the change: exact arity, extra arity, short arity, a varargs callee, and a typed `String[]` carrier all produce byte-identical results. That is a regression check on one JDK. It does not prove `Method.invoke` never writes to the array, an assumption the varargs branch has made since this file was copied from parquet-common. ``` build/mvn -o test -pl kyuubi-util -am -DwildcardSuites=none -Dtest=DynMethodsTest build/mvn -o spotless:check -pl kyuubi-util ``` 11 of 11 green and spotless clean. ### Was this patch assisted by generative AI tooling? Assisted-by: Claude Opus 5 -- 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]
