LuciferYang opened a new pull request, #7691: URL: https://github.com/apache/kyuubi/pull/7691
### Why are the changes needed? Closes #7690. `DynFields` is vendored from iceberg-common and its javadoc came across with the `DynMethods` wording still in it, so a field API ends up documented in method vocabulary. `bind` said it returns a `BoundMethod`, a type this file does not contain. Six `@throws` tags said "if the method is static" or "if the method is not static" for a check that reads `Modifier.isStatic` on a `Field`. Three `@param fieldName` tags carried `(different from constructor)`, which separates a method from a constructor and has nothing to separate on a field. Two of the fourteen corrections change an exception type. `build()` was documented as throwing `NoSuchFieldException` when it has no `throws` clause and raises `RuntimeException`; the `@throws RuntimeException` tag in the same comment already contradicted that summary. `buildChecked(Object)` was documented as returning a `BoundMethod` and throwing `NoSuchMethodException` while its signature declares `throws NoSuchFieldException`. In both cases the documented exception is one the compiler will not let a caller catch, so the failure path the javadoc described could not be written at all. The `UnboundField` class doc claimed "all Exceptions wrapped by RuntimeException, or with a single Exception catch block". `get` and `set` catch only `IllegalAccessException`, and this class has no checked-throwing accessor for the second clause to refer to; that clause describes `DynMethods.UnboundMethod.invokeChecked`. The class doc now names the one throwable that is wrapped and says the rest are not. Upstream iceberg-common carries the same wrong text, so this is not local drift that a resync would repair. Thirteen of the fourteen lines have a direct upstream counterpart and are worth sending to iceberg separately. Left for a follow-up: the builder's `@throws` tags are written unconditionally and stop holding once `defaultAlwaysNull()` is set, because the sentinel is returned instead of an exception and its `bind` override skips the static and receiver checks. Both `hiddenImpl` overloads also point `@see` at `Class#getField(String)` while the code calls `getDeclaredField`. `DynMethods` has both defects too, so the two vendored copies should change together. ### How was this patch tested? Every changed line sits inside a `/** */` block, so there is no behaviour to test, and a javadoc assertion would pass with the patch reverted. `build/mvn -o test -pl kyuubi-util -am -DwildcardSuites=none` gives 33/33 green. `build/mvn -o spotless:check -pl kyuubi-util` is clean. Existing tests in `DynFieldsTest` already cover the behaviour these sentences describe: `testBuildWithoutAlwaysNullFallbackThrows` for `build()` raising `RuntimeException`, `testBindRejectsStaticField` and `testBindRejectsIncompatibleTarget` for `bind`'s two conditions, and `testAlwaysNullBindsThroughEveryEntryPoint` for the sentinel path behind the deferred wording. Two of the behavioural claims have no test and were checked by reading the code: the class doc's wrapping sentence, and `asStatic`'s "if the field is not static" path. ### 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]
