LuciferYang commented on PR #17045: URL: https://github.com/apache/iceberg/pull/17045#issuecomment-5386618747
Drive-by from someone who was reading a vendored copy of this file. The bug is still there on main (checked at `f0e7de3c2`), and the override here matches how `DynMethods.UnboundMethod.NOOP` already solves the same problem, so the shape looks right to me. For context, not as a second bug report: Apache Kyuubi vendors a copy of `DynFields` in its `kyuubi-util` module and has the same defect. It is latent there, since nothing in that codebase calls `defaultAlwaysNull()`, so no user can hit it. A fix is open at apache/kyuubi#7652 and not merged yet; it landed on the same `bind` override as this PR, arrived at before I found this one. Two observations from reading it, in case they are useful here: - Once `AlwaysNull` overrides `bind`, the `this == AlwaysNull.INSTANCE` term in `UnboundField.bind`'s `checkState` becomes unreachable. It is also what turns the failure into an NPE at the following `checkArgument` rather than a clean `IllegalStateException`. `DynMethods.UnboundMethod.bind` carries no equivalent term, so dropping it would line the two classes up. - The sentinel is a no-op only for reads and for `set(null)`. Since `AlwaysNull` is an `UnboundField<Void>`, the bridge generated for `set(Object, Void)` casts the argument to `Void`, so `BoundField.set(someNonNullValue)` throws `ClassCastException`. Not something this PR needs to solve, but making `bind` work widens the path that reaches it. -- 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]
