vlsi commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r763336873
##########
File path: 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:
> So this test is correct
`VALUES is pushed down` can't be correct as it is unclear **why** it should
be pushed.
The test could be created for one of the following:
a) "SQL with VALUES doesn't result in a runtime error"
b) "this specific SQL pushes VALUES to JDBC because..."
c) "this specific SQL does NOT push VALUES to JDBC because..."
I think the test case is more like "doesn't result in a runtime error", so
`VALUES is pushed down` comment above is misleading.
If someone adjusts the costing model, the planner would realize pushing the
values to JDBC is less efficient (e.g. it produces more network traffic, it
consumes more CPU), so the "proper" behavior should be "values is NOT pushed to
JDBC".
----
If you want to test that "VALUES can be pushed at all", then the test case
should better include some join condition, so nobody questions the efficiency
of the pushing VALUES.
---
I don't really like how this discussion consumes time, however, you
suggested "update the comment" instead of removing it, and I clarify that the
comments (both old and new) are misleading. They add no value, and even the
test method name could convey the same (or better) message.
--
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]
