bkietz commented on code in PR #40687:
URL: https://github.com/apache/arrow/pull/40687#discussion_r1534204113
##########
cpp/src/arrow/compute/exec.cc:
##########
@@ -336,7 +336,7 @@ bool CheckIfAllScalar(const ExecBatch& batch) {
return false;
}
}
- return batch.num_values() > 0;
+ return batch.num_values() >= 0;
Review Comment:
```suggestion
return true;
```
##########
cpp/src/arrow/compute/function.cc:
##########
@@ -426,6 +426,7 @@ Status ScalarFunction::AddKernel(ScalarKernel kernel) {
if (arity_.is_varargs && !kernel.signature->is_varargs()) {
return Status::Invalid("Function accepts varargs but kernel signature does
not");
}
+ kernel.is_pure = is_pure();
Review Comment:
I've often thought about adding `const Function *Kernel::function` as a non
owning reference from the kernel to its function, which would be a larger
change but perhaps less of a special case than duplicating this one property.
As a side benefit, it'd allow us to remove the [bound call expression property
`function`](https://github.com/bkietz/arrow/blob/46758bc3c6321d8b7013acf52bff7761a8b33eda/cpp/src/arrow/compute/expression.h#L55)
@lidavidm @pitrou what do you think?
--
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]
