andygrove commented on code in PR #5216:
URL: https://github.com/apache/datafusion-comet/pull/5216#discussion_r3969212603
##########
spark/src/main/scala/org/apache/spark/sql/comet/DecimalPrecision.scala:
##########
@@ -40,6 +40,24 @@ import org.apache.spark.sql.types.DecimalType
*/
object DecimalPrecision {
def promote(expr: Expression): Expression = {
+ // `transformUp` walks and rebuilds every node even when no case matches,
and the serde calls
+ // this once per expression it converts, so skip it when there is nothing
to rewrite.
Review Comment:
Reworded in 4e0502ef3 — you're right, "rebuilds every node" overstates it
and points a future profiler at the wrong thing.
```scala
// `transformUp` walks the whole tree and calls `mapChildren` at every node,
which allocates
// through `children.map` / `mapProductIterator` even when no case matches
(the specialized
// `mapChildren` overrides do skip the node copy via `fastEquals`, so the
cost is the traversal
// and its allocations, not a rebuild). The serde calls this once per
expression it converts,
// so skip it when there is nothing to rewrite.
```
That now says the same thing as the PR body, and naming `fastEquals`
explicitly should stop the next reader concluding there is a copy to eliminate.
--
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]
