What is the problem the feature request solves?
QueryPlanSerde.exprToProto calls DecimalPrecision.promote over the complete expression tree before delegating to exprToProtoInternal. Some expression serdes then call the public exprToProto entry point again for children of that already-promoted tree. Each recursive public call starts another promotion traversal over the child subtree even though the root entry has already promoted it.
PR #5225 exposed the correctness consequence: repeated promotion must be idempotent, otherwise nested decimal arithmetic can acquire duplicate CheckOverflow wrappers. That PR makes promotion idempotent, but the redundant traversals remain.
There are currently 64 exprToProto( references under spark/src/main/scala/org/apache/comet/serde, across QueryPlanSerde.scala, aggregates.scala, arrays.scala, bitwise.scala, and operator serde files. Not all should change: some calls serialize independent roots or newly synthesized expressions and still need the public promotion entry point.
This overlaps finding 4 in #5199, but specifically tracks the recursive-entry-point audit and the contract between the public and internal serializers.
Describe the potential solution
- Audit the public
exprToProto calls in expression serdes.
- Use
exprToProtoInternal, or an equivalent no-promotion child path, when serializing a child that is already part of the promoted root tree.
- Keep public
exprToProto for independent roots and synthesized expression trees that have not been promoted.
- Document the ownership contract between the two entry points.
- Add a nested decimal-arithmetic regression that verifies recursive serialization retains exactly one equivalent
CheckOverflow wrapper.
Additional context
What is the problem the feature request solves?
QueryPlanSerde.exprToProtocallsDecimalPrecision.promoteover the complete expression tree before delegating toexprToProtoInternal. Some expression serdes then call the publicexprToProtoentry point again for children of that already-promoted tree. Each recursive public call starts another promotion traversal over the child subtree even though the root entry has already promoted it.PR #5225 exposed the correctness consequence: repeated promotion must be idempotent, otherwise nested decimal arithmetic can acquire duplicate
CheckOverflowwrappers. That PR makes promotion idempotent, but the redundant traversals remain.There are currently 64
exprToProto(references underspark/src/main/scala/org/apache/comet/serde, acrossQueryPlanSerde.scala,aggregates.scala,arrays.scala,bitwise.scala, and operator serde files. Not all should change: some calls serialize independent roots or newly synthesized expressions and still need the public promotion entry point.This overlaps finding 4 in #5199, but specifically tracks the recursive-entry-point audit and the contract between the public and internal serializers.
Describe the potential solution
exprToProtocalls in expression serdes.exprToProtoInternal, or an equivalent no-promotion child path, when serializing a child that is already part of the promoted root tree.exprToProtofor independent roots and synthesized expression trees that have not been promoted.CheckOverflowwrapper.Additional context