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/09/10 18:52:29 UTC

[PR] test: name the whole dispatched subtree in the decimal promotion assertion [datafusion-comet]

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

   ## Which issue does this PR close?
   
   Closes #5848.
   
   ## Rationale for this change
   
   `CometCodegenSuite."codegen dispatch coverage survives the decimal promotion rewrite"` fails on main and takes the `[expressions]` job down on all five Spark profiles, so every PR opened against main is red for a reason unrelated to its own changes:
   
   ```
   Some(Set("hypot", "cast", "checkoverflow", "add")) did not contain Set("hypot")
   (CometCodegenSuite.scala:431)
   ```
   
   The behaviour is correct and the assertion is stale. #5610 deliberately started naming every expression in a dispatched subtree rather than only its root: the whole subtree is bound and closure-serialized into one kernel, so all of it ran in the JVM, and naming only the root would let a test assert that a nested `abs` was native while an `abs` was in fact running inside the kernel. That PR regenerated the q78 plan-stability goldens for the wider dispatcher list but did not update this assertion, which predates it.
   
   ## What changes are included in this PR?
   
   Expect the promoted subtree's full set in that assertion: `hypot`, its `cast`, the `checkoverflow` wrapper decimal promotion adds around the `add`, and the `add`. The surrounding comment now says why the set has more than the root in it, and why `checkoverflow` appearing is the thing that makes the assertion evidence that the promoted copy, not the original tree, was recorded.
   
   No production code changes.
   
   ## How are these changes tested?
   
   `CometCodegenSuite` passes in full (96 tests) on Spark 4.1. The failing assertion was confirmed red on a clean checkout of main first, and the actual set is identical on all five profiles in CI (3.4, 3.5, 4.0, 4.1, 4.2), so no version-specific expectation is needed.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   
   https://claude.ai/code/session_01BtAqq4YJsk8uk42c7vHk8B
   


-- 
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] test: name the whole dispatched subtree in the decimal promotion assertion [datafusion-comet]

Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #5849:
URL: https://github.com/apache/datafusion-comet/pull/5849


-- 
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