dmitry-chirkov-dremio commented on code in PR #48597:
URL: https://github.com/apache/arrow/pull/48597#discussion_r3581619307
##########
cpp/src/gandiva/node.h:
##########
@@ -117,6 +121,10 @@ class GANDIVA_EXPORT FieldNode : public Node {
return "(" + field()->type()->ToString() + ") " + field()->name();
}
+ std::string ToCacheKeyString() const override {
+ return "(" + field()->type()->ToString() + ") ";
Review Comment:
Dropping the field name from the cache-key string looks unsafe. It makes
structurally different expressions like `add(a, a)` and `add(a, b)` collapse to
the same cache key whenever the types match, even though Gandiva will build
different input-descriptor layouts for those two trees.
Because the annotator interns fields by name, `add(a, a)` reuses one
descriptor while `add(a, b)` creates two distinct descriptors. Reusing cached
object code across those cases can bind the generated JIT to the wrong buffer
slots and return incorrect results. I think the cache key still needs to
distinguish repeated-reference shape somehow, even if we want to relax
schema/name matching.
--
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]
