bkietz commented on code in PR #14659:
URL: https://github.com/apache/arrow/pull/14659#discussion_r1024293201
##########
cpp/src/arrow/compute/exec/expression.cc:
##########
@@ -856,6 +870,27 @@ bool IsBinaryAssociativeCommutative(const
Expression::Call& call) {
return it != binary_associative_commutative.end();
}
+Result<Expression> HandleInconsistentTypes(Expression::Call call,
+ compute::ExecContext* exec_context)
{
+ // ARROW-18334: due to reordering of arguments, the call may have
+ // inconsistent argument types. For example, the call's kernel may
+ // correspond to `timestamp + duration` but the arguments happen to
+ // be `duration, timestamp`. The addition itself is still commutative,
+ // but the mismatch in declared argument types is potentially problematic
+ // if we ever start using the Expression::Call::kernel field more than
+ // we do currently. Check and rebind if necessary.
+ //
+ // The more correct fix for this problem is to ensure that all kernels of
+ // functions which are commutative be commutative as well, which would
+ // obviate rebinding like this. In the context of ARROW-18334, this
+ // would require rewriting KernelSignature so that a single kernel can
+ // handle both `timestamp + duration` and `duration + timestamp`.
+ if (call.kernel->signature->MatchesInputs(GetTypes(call.arguments))) {
Review Comment:
Well, that was my intent and the doc for each of these passes states that
the argument should be bound... but I guess we're not specifically checking.
I'll add a check to Canonicalize and FoldConstants
--
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]
