andygrove commented on PR #5714: URL: https://github.com/apache/datafusion-comet/pull/5714#issuecomment-5553599223
Pushed a review pass. One finding was a real limitation rather than a cleanup, so flagging it here: **`canHandle`'s `spark.sql.codegen.maxFields` gate counted `BoundReference` occurrences, not distinct ordinals.** The fused struct puts the shared deserializer in every field deliberately, so a 12-column typed map counted 12 + 12*12 = 156 against the default cap of 100 and silently declined to fuse. The feature capped out around 9 columns and nothing caught it, since the widest record in the suite had 4 fields. Fixed at the root: WSCG gates on the operator's schema, where a column read twice contributes once, and the kernel likewise emits one typed field and one getter per ordinal. Now pinned by a 12-column test that I confirmed fails without the fix. Note this touches shared code, so it also slightly relaxes the gate for `ScalaUDF` — in the correct direction. Also worth a reviewer's attention, and deliberately **not** changed here: **This rewrite could live in `convertNode` instead of as an `_apply` pre-pass, and probably should.** My original comment justified the pre-pass by claiming the bottom-up walk would have already fallen the island back — that is wrong, and I have corrected it. `convertNode` only tags `DeserializeToObjectExec` with a fallback reason; it never replaces it, so the sandwich is structurally intact when the walk reaches `SerializeFromObjectExec`. Doing the fusion there would let the rule see whether `deserialize.child` actually converted to a `CometNativeExec` and decline when it did not — which is exactly the profitability signal this rewrite currently lacks and the reason the config is off by default. That is a ~55 line change and a behavior change (it would decline more often), so I left it out of a cleanup pass, but it is the thing most likely to let the flag default to `true`. The rest is cleanup: extracted `CometScalaUDF.canDispatch` (the rule was re-deriving the bind-then-`canHandle` gate and had already drifted — it omitted the `RuntimeReplaceable` unwrap); moved `FORCE_DISPATCH` onto `QueryPlanSerde` which reads it, matching what every other behavior tag in the codebase does, and documented that the tag bypasses the per-expression policy layer; moved the tag check ahead of the version shim so the override is unconditional; moved the pre-pass above `normalizePlan` so the synthesized projections get the same NaN / -0.0 treatment as every other `ProjectExec`. On tests: one assertion was vacuous — `Dataset.schema` comes from the analyzed plan, which a physical rule cannot touch — so it now compares the executed plan's output attributes instead. Both decline paths now pin their message via `checkSparkAnswerAndFallbackReason` rather than just asserting the sandwich survived. 17 tests, green on the default profile and `-Pspark-3.5`; compiles on 3.4/3.5/4.0/default. No regressions in `CometCodegenSuite` (86), `CometCodegenSourceSuite` (60), `CometCodegenFuzzSuite` (28), `CometExpressionSuite` (141), `CometExecRuleSuite` (29). -- 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]
