andygrove commented on PR #5593: URL: https://github.com/apache/datafusion-comet/pull/5593#issuecomment-5607529640
Moving this to draft. I merged main and re-ran the noise check, and the result made me doubt the premise rather than confirm it. The warning now fires three times across `CometExpressionSuite` and `CometArrayExpressionSuite` (~205 tests), and I no longer think any of the three is worth acting on. The message tells the reader to report the case from `getSupportLevel` as `Unsupported` or `Incompatible` so that a `CodegenDispatchFallback` serde can route it through the codegen dispatcher — but that only does something when the handler actually mixes in `CodegenDispatchFallback`, which is the one thing `dispatchIfFallback` tests. `CometCreateMap` is a `CometCodegenDispatch`, `CometScalaUDF` is a plain `CometExpressionSerde`, and `CometInitCap` is a `CometScalarFunction with NativeOptInAvailable`. None of them is one. Take the advice on any of the three and `dispatchIfFallback` no-ops, control falls through to `withFallbackReason(...); None`, and the user gets the identical Spark fallback with no warning attached. Nothing was lost, so there was nothing to report. `CometCreateMap` is the clearest case and the one that changed my mind. Its `convert` delegates straight to the dispatcher, and the dispatcher is what declined — the output type has two struct fields named `x`, which it cannot write. Telling that serde to report `Unsupported` so the dispatcher can have a go is circular. `CometCodegenDispatch` backs around 62 serdes, so that circular shape is the dominant source of warnings here rather than an edge case. The rationale in the description is still right as far as it goes: a `Compatible` serde that declines inside `convert` does skip a dispatcher attempt. I had the population wrong. It only costs anything when the serde is a `CodegenDispatchFallback`, and the check I added does not test for that, which is why it fires exclusively on cases where the dispatcher was never reachable to begin with. The narrow fix is to gate the warning on `handler.isInstanceOf[CodegenDispatchFallback]`. That is one line, and it would have suppressed all three. What stops me merging is what is left afterwards: silence across all 205 tests. The argument for landing this ahead of #5574 was that it would let the cleanup be scoped from evidence, and with the gate in place the evidence is empty. It stops being a diagnostic and becomes a guard against a violation I have not shown exists — and if that is all it is, a `logWarning` in the planner is the wrong home for it, since an assertion in `CometSerdeInvariantSuite` gets the same protection without shipping a warning nobody can act on. So what decides this is whether a real instance exists: a `CodegenDispatchFallback` serde that reports `Compatible` and then declines in `convert` for a reason depending only on the node. There are 32 serdes mixing that trait in — `CometCast`, `CometLike`, `CometConcatWs`, `CometRound`, `CometSequence`, `CometStringLPad`/`RPad` and the rest. I want to work through those before deciding. If one turns up, I will add the gate and put this back up for review on its original terms. If none does, I would rather close this and fold the invariant into #5574 as a test than merge a warning that has only ever fired on non-problems. -- 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]
