LuciferYang opened a new pull request, #7652: URL: https://github.com/apache/kyuubi/pull/7652
### Why are the changes needed? Closes #7651. `DynFields.AlwaysNull` is a singleton with a `null` backing `Field`. It did not override `bind(Object)`, so `build(target)` / `buildChecked(target)` fell through to `UnboundField.bind`, which exempts the sentinel from the static-field check and then dereferences the null field on the next check, throwing `NullPointerException` instead of returning a field that reads `null`. The fix mirrors `DynMethods.UnboundMethod.NOOP`, the sentinel in the sibling class, which already overrides `bind` to return a bound no-op. With that override in place the `this != AlwaysNull.INSTANCE` term in `UnboundField.bind` is unreachable, so it is narrowed to `if (isStatic())` and the sentinel case lives in one place only. `DynMethods.UnboundMethod.bind` has exactly that shape. Two things stated so the change is not read as more than it is: - Nothing in Kyuubi calls `defaultAlwaysNull()` today, so the NPE is currently unreachable. This hardens a vendored utility rather than fixing a reported failure. - The sentinel is a no-op for reads and for `set(null)` only. Because `AlwaysNull` is an `UnboundField<Void>`, the bridge generated for `set(Object, Void)` casts the argument to `Void`, so `BoundField.set(nonNull)` throws `ClassCastException`. That is pre-existing, reachable before this patch through `buildStatic().set(x)`, and not addressed here: making writes a true no-op would mean changing the sentinel's type parameter and its `set` contract, which is a separate concern and belongs upstream first. `kyuubi-util`'s `DynFields` is copied from `iceberg-common`, which carries the same defect. It is tracked upstream as apache/iceberg#17044 and unfixed as of iceberg 1.11.0. The proposed upstream patch, apache/iceberg#17045, adds the same override but keeps the sentinel exemption in `UnboundField.bind`, so that one line stays different in this copy even after upstream lands. ### How was this patch tested? New `DynFieldsTest`, 11 cases, covering the sentinel through every entry point (`build`, `build(target)`, `buildChecked(target)`, `buildStatic`, `buildStaticChecked`, direct `bind`) and the ordinary static/instance field paths: ``` build/mvn test -pl kyuubi-util -am -Dtest=DynFieldsTest -DwildcardSuites=none ``` ``` Tests run: 11, Failures: 0, Errors: 0, Skipped: 0 ``` Mutation-checked that the new cases have teeth: - restoring `DynFields.java` to its state before this patch: 3 cases fail with `NullPointerException: Cannot invoke "java.lang.reflect.Field.getDeclaringClass()" because "this.field" is null` - removing the `AlwaysNull.toString()` override: `testAlwaysNullToStringReportsSentinelName` fails with the same NPE, and only that case `build/mvn spotless:check -pl kyuubi-util` passes. ### Was this patch assisted by generative AI tooling? Assisted-by: Ducc: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]
