andygrove commented on code in PR #5225:
URL: https://github.com/apache/datafusion-comet/pull/5225#discussion_r3738222125
##########
spark/src/main/scala/org/apache/spark/sql/comet/DecimalPrecision.scala:
##########
@@ -45,6 +45,15 @@ object DecimalPrecision {
// happen if the Spark version is < 3.4
case e: BinaryArithmetic if e.left.prettyName == "promote_precision" => e
Review Comment:
Now that the serde no longer adds its own wrapper, `promote` is the only
thing standing between a decimal `Divide` and the raw `i128::MAX` sentinel.
That makes this branch worth a second look, because it is the one arm that
returns early without wrapping.
The comment says it can only fire on Spark versions below 3.4, and
`QueryPlanSerde.scala:911` says the same thing about 3.3. `PromotePrecision`
was removed by SPARK-39316 in 3.4.0, and `pom.xml` puts the floor at 3.4.3, so
this cannot match on any version Comet builds against. It is also the one
remaining shape where a decimal `Divide` reaches `CometDivide.convert` with
both operands decimal and no `CheckOverflow` anywhere, which is exactly the
silent-garbage case the new `getSupportLevel` guard was added to rule out. That
guard cannot catch it, since the operands are decimal there.
Could this branch just be deleted? It is three lines, and removing it makes
the invariant total rather than nearly total. If you would rather not touch an
unrelated arm of the rule in this PR, a tracking issue linked here works too. I
would just not leave it as-is, now that it is load-bearing in a way it was not
before.
--
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]
