You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "l4wei (via GitHub)" <gi...@apache.org> on 2024/01/08 13:02:00 UTC
[PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
l4wei opened a new pull request, #3616:
URL: https://github.com/apache/calcite/pull/3616
…use of wrong parent relationship when replace query
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-2524847425
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "JiajunBernoulli (via GitHub)" <gi...@apache.org>.
JiajunBernoulli commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-1891349040
Can you consider reusing existing code as much as possible?
You just provided a workaround to solve 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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3616:
URL: https://github.com/apache/calcite/pull/3616#discussion_r1529075316
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -649,6 +661,96 @@ assert rowTypesAreEquivalent(
return substitutions;
}
+ /**
+ * Find out the replacements belongs to the real substitution and undo the rest.
+ * The rest replacements are exploratory, they are no more needed when this attempt finished.
+ */
+ private List<Replacement> undoAndRemoveExploratoryReplacements(
+ List<Replacement> attempted, List<MutableRel> targetDescendants,
+ MutableRel replacement, boolean lastIsFinalReplacement) {
+ final List<Replacement> realReplacements = new ArrayList<>();
+ final List<Replacement> exploratoryReplacements =
+ new ArrayList<>(lastIsFinalReplacement
+ ? attempted.subList(0, attempted.size() - 1)
+ : attempted);
+ findOutRealReplacements(
+ exploratoryReplacements,
+ realReplacements,
+ null,
+ targetDescendants,
+ replacement);
+ Collections.reverse(realReplacements);
+ exploratoryReplacements.removeAll(realReplacements);
+ undoReplacement(exploratoryReplacements);
+ if (lastIsFinalReplacement) {
+ realReplacements.add(attempted.get(attempted.size() - 1));
+ }
+ return realReplacements;
+ }
+
+ private void findOutRealReplacements(
Review Comment:
Rename findOutRealReplacements -> findRealReplacements() (or maybe identifyRealReplacements())
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-1882484676
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[94.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3616:
URL: https://github.com/apache/calcite/pull/3616#discussion_r1529081391
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -649,6 +661,96 @@ assert rowTypesAreEquivalent(
return substitutions;
}
+ /**
+ * Find out the replacements belongs to the real substitution and undo the rest.
+ * The rest replacements are exploratory, they are no more needed when this attempt finished.
+ */
+ private List<Replacement> undoAndRemoveExploratoryReplacements(
+ List<Replacement> attempted, List<MutableRel> targetDescendants,
+ MutableRel replacement, boolean lastIsFinalReplacement) {
+ final List<Replacement> realReplacements = new ArrayList<>();
+ final List<Replacement> exploratoryReplacements =
+ new ArrayList<>(lastIsFinalReplacement
+ ? attempted.subList(0, attempted.size() - 1)
+ : attempted);
+ findOutRealReplacements(
+ exploratoryReplacements,
+ realReplacements,
+ null,
+ targetDescendants,
+ replacement);
+ Collections.reverse(realReplacements);
+ exploratoryReplacements.removeAll(realReplacements);
+ undoReplacement(exploratoryReplacements);
+ if (lastIsFinalReplacement) {
+ realReplacements.add(attempted.get(attempted.size() - 1));
+ }
+ return realReplacements;
+ }
+
+ private void findOutRealReplacements(
+ List<Replacement> src, List<Replacement> result, @Nullable Replacement last,
+ List<MutableRel> targetDescendants, MutableRel replacement) {
+ if (src == null || src.isEmpty()) {
+ return;
+ }
+ final Replacement current = src.get(src.size() - 1);
+ if (last == null || isPrevReplacement(current, last)) {
+ result.add(current);
+ last = current;
+ }
+ if (isFirstReplacement(current, targetDescendants, replacement)) {
+ return;
+ }
+ findOutRealReplacements(
+ src.subList(0, src.size() - 1),
+ result,
+ last,
+ targetDescendants,
+ replacement);
+ }
+
+ /**
+ * Return true when x replacement is the previous one of y.
+ */
+ private boolean isPrevReplacement(Replacement x, Replacement y) {
+ final MutableRel prev = x.after;
+ final MutableRel next = y.before;
+ if (prev == next) {
+ return true;
+ }
+ final MutableRel prevParent = prev.getParent();
+ assert prevParent != null;
+ for (MutableRel cur : MutableRels.descendants(next)) {
+ if (cur == prevParent) {
+ return true;
+ }
+ }
+ return false;
+ }
+
+ /**
+ * Return true when x is the first one of real replacements chain,
+ * which the before of it is not the part of the targetDescendants and is not replacement.
+ */
+ private boolean isFirstReplacement(
+ Replacement r,
+ List<MutableRel> targetDescendants,
+ MutableRel replacement) {
+ final MutableRel before = r.before;
+ for (MutableRel cur : MutableRels.descendants(before)) {
+ if (replacement.equals(cur)) {
+ return false;
+ }
+ for (MutableRel targetDescendant : targetDescendants) {
Review Comment:
Does this need to descend again to MutableRels.descendants(targetDescendant)?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-1881004117
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[94.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-2137072730
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616) **Quality Gate passed**
Issues
 [1 New issue](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3616&resolved=false&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3616&resolutions=WONTFIX)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3616&resolved=false&sinceLeakPeriod=true)
 [89.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "l4wei (via GitHub)" <gi...@apache.org>.
l4wei commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-1895388190
> Can you consider reusing existing code as much as possible?
>
> You just provided a workaround to solve it.
I don't think so, I think the real problem is that exploratory replacements have not been undo, so I fix it, this is not a workaround.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-4465614318
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 90 days if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@calcite.apache.org list. Thank you for your contributions.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-1895455901
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[92.2% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3616:
URL: https://github.com/apache/calcite/pull/3616#issuecomment-1891258665
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616) **Quality Gate passed**
The SonarCloud Quality Gate passed, but some issues were introduced.
[3 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3616&resolved=false&inNewCodePeriod=true)
[94.4% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_coverage&view=list)
[0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3616&metric=new_duplicated_lines_density&view=list)
[See analysis details on SonarCloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3616)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-6193] SubstitutionVisitor stop trying incorrect subtree beca… [calcite]
Posted by "jduo (via GitHub)" <gi...@apache.org>.
jduo commented on code in PR #3616:
URL: https://github.com/apache/calcite/pull/3616#discussion_r1529076769
##########
core/src/main/java/org/apache/calcite/plan/SubstitutionVisitor.java:
##########
@@ -649,6 +661,96 @@ assert rowTypesAreEquivalent(
return substitutions;
}
+ /**
+ * Find out the replacements belongs to the real substitution and undo the rest.
+ * The rest replacements are exploratory, they are no more needed when this attempt finished.
+ */
+ private List<Replacement> undoAndRemoveExploratoryReplacements(
+ List<Replacement> attempted, List<MutableRel> targetDescendants,
+ MutableRel replacement, boolean lastIsFinalReplacement) {
+ final List<Replacement> realReplacements = new ArrayList<>();
+ final List<Replacement> exploratoryReplacements =
+ new ArrayList<>(lastIsFinalReplacement
+ ? attempted.subList(0, attempted.size() - 1)
+ : attempted);
+ findOutRealReplacements(
+ exploratoryReplacements,
+ realReplacements,
+ null,
+ targetDescendants,
+ replacement);
+ Collections.reverse(realReplacements);
+ exploratoryReplacements.removeAll(realReplacements);
+ undoReplacement(exploratoryReplacements);
+ if (lastIsFinalReplacement) {
+ realReplacements.add(attempted.get(attempted.size() - 1));
+ }
+ return realReplacements;
+ }
+
+ private void findOutRealReplacements(
+ List<Replacement> src, List<Replacement> result, @Nullable Replacement last,
+ List<MutableRel> targetDescendants, MutableRel replacement) {
+ if (src == null || src.isEmpty()) {
+ return;
+ }
+ final Replacement current = src.get(src.size() - 1);
+ if (last == null || isPrevReplacement(current, last)) {
+ result.add(current);
+ last = current;
+ }
+ if (isFirstReplacement(current, targetDescendants, replacement)) {
+ return;
+ }
+ findOutRealReplacements(
+ src.subList(0, src.size() - 1),
Review Comment:
Is this going to produce alot of stack frames with large inputs?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org