andygrove commented on PR #5773: URL: https://github.com/apache/datafusion-comet/pull/5773#issuecomment-5589300779
Thanks for the review. Both P2 findings were real and are fixed in 85bd18354; I replied on each thread with the specifics. On the validation gap you raised: you are right that the test established only support levels, and I want to be straight about how far I closed that rather than implying it is now covered end to end. **What I added.** The test now asserts the serialized call (`convert` produces an `iceberg_bucket` scalar func), not just the support level. That is the part that was actually untested: `ApplyFunctionExpression` carries its arguments and result type on `children` / `dataType`, whereas the `StaticInvoke` path reads `arguments` / `dataType`. The support-level assertion alone would have passed even if the new `unwrap` seam extracted the wrong thing, since `getSupportLevel` only counts and inspects the arguments while `convert` is what serializes them. I also added a local `UnlistedScalarFunction` double, a DSv2 function with no magic method and no Comet handler, to pin that an unhandled class stays on Spark instead of matching the Iceberg handlers by argument shape alone. **What I did not add, and why.** There is still no catalog-resolution or native-execution test for this lowering, and I do not think one is reachable in this repo's matrix. Every Iceberg runtime we build against (1.5.2, 1.8.1, 1.10.0, 1.11.0) ships the static magic method on these classes, so `V2ExpressionUtils.resolveScalarFunction` will always pick `StaticInvoke` for them. Routing a real query through `ApplyFunctionExpression` would need either an Iceberg build that omits the magic method or a delegating wrapper, and a wrapper defeats itself: the handler map is keyed by class name, so the wrapper's own name would not match. Your point that `BucketInt` already has a magic method is exactly the reason, not an oversight in the test. If you would rather gate merge on execution coverage, the honest options are a hand-built fixture jar or leaving this path unit-tested, and I would take your read on which is worth it. **On the sixth CI failure.** Agreed it is not attributable to this change, and I pulled the log to confirm rather than infer it from the step name: the job reports `Tests: succeeded 477, failed 0` and then fails on `Failed to FinalizeArtifact: ... (403) Forbidden` during upload. Local runs of `CometIcebergSystemFunctionSuite`: 13 succeeded, 0 failed, 0 canceled on both the default profile (Spark 4.1, Scala 2.13) and `-Pspark-3.4` (Scala 2.12). CI on the new commit is still in flight, so I am not claiming the full matrix yet. -- 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]
