peter-toth commented on code in PR #38052:
URL: https://github.com/apache/spark/pull/38052#discussion_r985078417
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -2157,7 +2157,7 @@ class SubquerySuite extends QueryTest
}
}
- test("Merge non-correlated scalar subqueries from different parent plans") {
Review Comment:
Unfortunately, this is also a valid test and so it shouldn't be altered. The
2 leaf level subqueries are independent and can be merted to compute once.
Similarly the 2 parents can be also merged. Admittedly, this test case is not
the best, I should have used a different tables than `testData` in the parent
queries to make it more simple.
But I don't want to block this fix so if we need it urgently I'm ok with
mergint this PR but probably we can come up with a better alternative.
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -2157,7 +2157,7 @@ class SubquerySuite extends QueryTest
}
}
- test("Merge non-correlated scalar subqueries from different parent plans") {
Review Comment:
I think the issue is that currently we allow merging a subquery to another
one that is needed to compute the subquery itself. So before merging we should
collect the (transitive) subquery references and don't try merging the new plan
to those.
@dtenedor, let me know if you want to update this PR or I can open one with
the fix tomorrow.
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -2157,7 +2157,7 @@ class SubquerySuite extends QueryTest
}
}
- test("Merge non-correlated scalar subqueries from different parent plans") {
Review Comment:
Unfortunately, this is also a valid test and so it shouldn't be altered. The
2 leaf level subqueries are independent and can be merted to compute once.
Similarly the 2 parents can be also merged. Admittedly, this test case is not
the best, I should have used a different table than `testData` in the parent
queries to make the test more simple.
But I don't want to block this fix so if we need it urgently I'm ok with
mergint this PR but probably we can come up with a better alternative.
##########
sql/core/src/test/scala/org/apache/spark/sql/SubquerySuite.scala:
##########
@@ -2157,7 +2157,7 @@ class SubquerySuite extends QueryTest
}
}
- test("Merge non-correlated scalar subqueries from different parent plans") {
Review Comment:
I think the issue is that currently we allow merging a subquery to another
one that is needed to compute the subquery itself. So we should collect the
(transitive) subquery references in the new plan and don't try merging the new
plan to those.
@dtenedor, let me know if you want to update this PR or I can open one with
the fix tomorrow.
--
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]
