clintropolis commented on code in PR #20149:
URL: https://github.com/apache/druid/pull/20149#discussion_r3907244716
##########
processing/src/main/java/org/apache/druid/segment/VirtualColumns.java:
##########
@@ -282,7 +282,14 @@ public VirtualColumn findEquivalent(Node otherNode)
toCheckForEquivalence =
otherVirtualColumn.rewriteRequiredColumns(equivalenceRewriteMap);
}
- return equivalence.get().get(toCheckForEquivalence.getEquivalanceKey());
+ VirtualColumn matched =
equivalence.get().get(toCheckForEquivalence.getEquivalanceKey());
+ if (matched != null &&
+ // guardrail check for expression collision when a virtual column
shadows the physical column
+ // e.x. otherNode v0 = dim1 and VCs dim1 = dim2 plus q = dim1, q can
be treated as equivalent to v0 even though it reads physical column dim2
+ getNode(matched.getOutputName()).getDependencies().size() ==
otherNode.getDependencies().size()) {
Review Comment:
this example won't be considered equivalent because the earlier loop first
finds all equivalent dependencies, which they are in this case, but then we
rewrite the comparing virtual columns dependencies with their equivalents,
which is where it fails (`concat(a,b)` != `concat(a,a)`). I think because the
earlier loop is confirming that all of otherNode's dependencies are equivalent,
as long as we make sure the equivalent doesn't have any 'extra' dependencies,
it should be fine.
--
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]
