jduo commented on code in PR #3616:
URL: https://github.com/apache/calcite/pull/3616#discussion_r1529070203
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -606,14 +607,25 @@ assert rowTypesAreEquivalent(
// replacement list and add them into substitution list.
// Meanwhile we stop matching the descendants and jump
// to the next subtree in pre-order traversal.
+ boolean lastIsFinalReplacement = false;
if (!target.equals(replacement)) {
Replacement r =
replace(query.getInput(), target, replacement.clone());
assert r != null
: rule + "should have returned a result containing the
target.";
attempted.add(r);
+ lastIsFinalReplacement = true;
}
- substitutions.add(ImmutableList.copyOf(attempted));
+ // Let attempted only contains replacements of real
substitution and the rest
+ // must undo. Otherwise, the rest (exploratory replacements)
will affect the next
+ // attempt, because all the replacements will let these
child nodes share the same
+ // targetDescendant, which they also share the same parent
node, it will make
+ // stopTrying be wrong when exploratory replacements still
exist.
+ final List<Replacement> realReplacements =
+ undoAndRemoveExploratoryReplacements(
+ attempted, targetDescendants, replacement,
lastIsFinalReplacement);
+ substitutions.add(ImmutableList.copyOf(realReplacements));
Review Comment:
Nit: Why not just add realReplacements directly to substitutions instead of
copying it then clearing it?
--
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]
