You are viewing a plain text version of this content. The canonical link for it is here.

Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2026/08/31 14:55:24 UTC

[PR] chore: warn when a Compatible serde declines in convert [datafusion-comet]

andygrove opened a new pull request, #5593:
URL: https://github.com/apache/datafusion-comet/pull/5593

## Which issue does this PR close?

Related to #5574.

That issue is about the dispatcher being unreachable from `convert`. This PR does not fix that — it adds the diagnostic that makes the affected cases visible, so the cleanup in #5574 can be scoped from evidence rather than from a static sweep.

## Rationale for this change

A serde whose `getSupportLevel` returns `Compatible` is promising that `convert` will succeed. When `convert` returns `None` instead, two things happen and neither is obvious:

- the enclosing operator falls back to Spark, and
- the JVM codegen dispatcher never gets a chance at the expression, because `dispatchIfFallback` is only reached from the `Unsupported` and `Incompatible` arms of `exprToProtoInternal` (`QueryPlanSerde.scala:941` and `:968`).

So a check that lives in `convert` instead of `getSupportLevel` silently costs the user a whole-operator fallback that the dispatcher could have absorbed. Today nothing surfaces that.

## What changes are included in this PR?

A warning on the `Compatible` arm when `convert` returns `None`, plus a new `CometSerdeInvariantSuite`.

The one piece of actual logic is the suppression rule. `convert` recurses into children, so a `Compatible` serde legitimately returns `None` when a *child* could not be serialized — `getSupportLevel` inspects the node, not the subtree. Without a guard, a single unsupported leaf would warn once for every ancestor on the way up. The warning therefore stays quiet when any strict descendant already carries a `FALLBACK_REASONS` tag, which is how a failed child announces itself. Note this deliberately checks *descendants only*: several serdes (`CometCreateArray`, `CometCreateNamedStruct`, `CometArraysZip`) also tag themselves when a child fails, so keying off the node's own tag would defeat the check.

Two categories warn without being a serde bug, and both are documented in the scaladoc:

- `getSupportLevel` is not given `inputs`, so an `inputs`-dependent decline cannot be moved there. `CometAttributeReference` is the only one today.
- With `spark.comet.exec.scalaUDF.codegen.enabled=false`, every serde that reports `Compatible` and then routes to `emitJvmCodegenDispatch` warns, because the disabled-config check lives in the dispatcher rather than in `getSupportLevel`. Those are genuine instances of the invariant violation; moving that check up belongs with #5574, not here.

## How are these changes tested?

New `CometSerdeInvariantSuite` (5 tests, registered in both PR workflows): warns on a real `Compatible`-then-`None` path, stays quiet when only a child failed, stays quiet when the parent tags itself *and* a child failed, warns when the node is the only tagged expression, and warns with a placeholder when no reason was recorded. The tests capture WARN output through a log4j2 appender attached to the `QueryPlanSerde` logger.

On noise, which was the main thing I wanted to check before proposing this: `CometArrayExpressionSuite` (57 tests) produced exactly one warning, from the test that disables the dispatcher config, and it is a true positive:

```
CometScalaUDF reported Compatible for UDF(knownnotnull(_4#1136)) but convert() returned None
(scalaudf: spark.comet.exec.scalaUDF.codegen.enabled=false; expression has no native path so
the plan falls back to Spark), so the enclosing operator falls back to Spark.
```

Under the default configuration it is silent.

--
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org

Re: [PR] chore: warn when a Compatible serde declines in convert [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.

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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] chore: warn when a Compatible serde declines in convert [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.

andygrove closed pull request #5593: chore: warn when a Compatible serde declines in convert
URL: https://github.com/apache/datafusion-comet/pull/5593


-- 
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: github-unsubscribe@datafusion.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org