zabetak commented on code in PR #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r890082281
##########
core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java:
##########
@@ -50,23 +50,25 @@
* same time. */
private static final ReentrantLock LOCK = new ReentrantLock();
- /** VALUES is not pushed down, currently. */
+ /** VALUES is pushed down. */
@Test void testValuesPlan() {
final String sql = "select * from \"days\", (values 1, 2) as t(c)";
- final String explain = "PLAN="
- + "EnumerableNestedLoopJoin(condition=[true], joinType=[inner])\n"
- + " JdbcToEnumerableConverter\n"
+ final String explain = "PLAN=JdbcToEnumerableConverter\n"
+ + " JdbcJoin(condition=[true], joinType=[inner])\n"
+ " JdbcTableScan(table=[[foodmart, days]])\n"
- + " EnumerableValues(tuples=[[{ 1 }, { 2 }]])";
+ + " JdbcValues(tuples=[[{ 1 }, { 2 }]])";
Review Comment:
I think that the discussion around the test naming, comments, and purpose is
not particularly crucial for this PR. I am not sure what was the purpose of
this test before the changes introduced in this PR, and I am not sure either
what is the purpose from now on going forward. Thus, I plan to accept the
latest changes which merely update the comment.
The **main point** I keep from this conversation is that the change in this
PR can potentially lead to important performance regression when a Cartesian
product is pushed into the JDBC backend. Nevertheless, as @julianhyde
mentioned, the decision to push a cartesian product or not should be part of
the cost model rather than the rule which defines valid transformations thus I
think it makes sense to go forward with the current PR as it is.
--
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]
