You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2021/11/28 18:41:13 UTC
[GitHub] [calcite] francesco-gini88 opened a new pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
francesco-gini88 opened a new pull request #2620:
URL: https://github.com/apache/calcite/pull/2620
…RexNode
--
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
[GitHub] [calcite] francesco-gini88 commented on pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
francesco-gini88 commented on pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#issuecomment-1081741773
hi there. This PR hasn't made the cut for one of the previous release. I'd like to complete the work, but I am unclear on what is left that I should be doing. There is an open thread around the efficiency of a plan generated by a join with `values`, can someone advise if any change is needed for the PR to go in or whether we don't want the change because of concern of plan efficiency, in which case I'd like to close the pr
--
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r771789797
##########
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:
is the Values pushed because
```
case LITERAL:
// literal on a join condition would be TRUE or FALSE
return true;
```
so that Values were treated as Literal?
If we have a strong argument here maybe only returns true when it is a boolean (true/false) and leave a TODO/Jira for whether pushing Values?
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r762877661
##########
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:
In practice, this becomes a less efficient plan, and it would be more efficient to "duplicate in-memory" rows rather than "duplicate rows at the database side, and spend extra network and CPU resources for processing the duplicates".
So this `VALUES` should not be pushed to Jdbc under a proper cost model.
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r771795344
##########
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:
I think This PR has addressed what is described in the Summary. And @vlsi comments are reasonable. So I support creating another Jira to add more unit tests about the VALUES is pushed down or not.
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r763322570
##########
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:
The JDBC rules' job is to apply all valid transformations. RelToSql's job is to convert a plan to SQL. Somewhere else there is a cost model that determines whether executing the whole query in the backend database is the optimal plan. So this test is correct.
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r760717021
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -402,6 +400,29 @@
+ "FROM \"SCOTT\".\"DEPT\") AS \"t1\" ON \"t0\".\"DEPTNO\" = \"t1\".\"DEPTNO\"");
}
+ @Test void testJoinConditionAlwaysTruePushDown() {
+ CalciteAssert.model(JdbcTest.SCOTT_MODEL)
+ .query(
+ "select empno, ename, d.deptno, dname\n"
+ + "from scott.emp e,scott.dept d\n"
+ + "where true")
+ .explainContains(
Review comment:
please match the formatting of strings in other tests. double quotes right after open parentheses.
--
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
[GitHub] [calcite] francesco-gini88 commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
francesco-gini88 commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r771798236
##########
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:
@amaliujia my understanding is that Values is pushed down because in that way the join is also pushed down. What's changed is that now the join can be pushed down, because it is a cross join and
```
case LITERAL:
// literal on a join condition would be TRUE or FALSE
return true;
```
the condition is always the literal true.
Therefore I figure the planner is picking the plan where the join is pushed down (generally speaking might be better to push down the join, although maybe not this particular case) and consequently the Values are also pushed down. For this reason I don't think a TODO belongs to that part of the code. I think a Jira describing exactly in which cases/queries Values should not be pushed down would be a better alternative.
I'm happy to add a join condition if that makes the test clearer.
```
select * from \"days\", (values 1, 2) as t(c) where \"day\" = t.c
```
A query like this one wouldn't be affected by my change (the join condition is not a literal) and the Values would still be pushed down
--
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
[GitHub] [calcite] vlsi commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
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..."
d) your option here?
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] vlsi commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r762877661
##########
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:
In practice, this becomes a less efficient plan, and it would be more efficient to "duplicate in-memory" rows rather than "duplicate rows at the database side, and spend extra network and CPU resources for processing the duplicates".
So in a proper cost model, this should not be pushed to Jdbc.
--
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
[GitHub] [calcite] amaliujia commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r771789797
##########
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:
is the Values pushed down because
```
case LITERAL:
// literal on a join condition would be TRUE or FALSE
return true;
```
so that Values were treated as Literal?
If we have a strong argument here maybe only returns true when it is a boolean (true/false) and leave a TODO/Jira for whether pushing Values?
--
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
[GitHub] [calcite] francesco-gini88 commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
francesco-gini88 commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r762537899
##########
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##########
@@ -332,6 +332,9 @@ protected JdbcJoinRule(Config config) {
* @return Whether condition is supported
*/
private static boolean canJoinOnCondition(RexNode node) {
+ if (node.isAlwaysTrue() || node.isAlwaysFalse()) {
+ return true;
+ }
Review comment:
shouldn't I check that then the literal is in fact a boolean?
```
case LITERAL:
return ((RexLiteral) node).getTypeName() == SqlTypeName.BOOLEAN;
```
or perhaps there is some guarantee that something that is not a boolean can't show up there?
--
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
[GitHub] [calcite] NobiGo commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
NobiGo commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r759737227
##########
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##########
@@ -332,6 +332,9 @@ protected JdbcJoinRule(Config config) {
* @return Whether condition is supported
*/
private static boolean canJoinOnCondition(RexNode node) {
+ if (node.isAlwaysTrue() || node.isAlwaysFalse()) {
+ return true;
+ }
Review comment:
Add another case condition:
```
// ***************
case LITERAL:
return true;
```
is more suitable ?
--
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
[GitHub] [calcite] francesco-gini88 commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
francesco-gini88 commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r771716821
##########
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:
FWIW I think the test and comment `VALUES is not pushed down, currently.` was giving the false impression that `Values` can't be pushed down into the db. A query like `select * from "days", (values 1, 2) as t(c) where t.c = "days"."day"` would have had a plan with VALUES pushed down
```
"PLAN=JdbcToEnumerableConverter\n"
+ " JdbcJoin(condition=[=($2, $0)], joinType=[inner])\n"
+ " JdbcTableScan(table=[[foodmart, days]])\n"
+ " JdbcValues(tuples=[[{ 1 }, { 2 }]])"
```
The reason why `VALUES` was not pushed down is because the cross join couldn't be pushed down in the first place.
On the point that the plan is less efficient and that it would be more efficient to "duplicate in-memory" rows. I can see where your observation is coming from. However, this test case is quite simple. In reality I would expect other aspects to matter as well. For instance in the in memory implementation of the cross join I _think_ that the inner iterator is instanciated multiple times. If the inner iterator happens to be `JdbcToEnumerableConverter` that would equate to running a query multiple times, because the inner iterator is created multiple times. This would mean fetching rows multiple times plus adding latency multiple times.
In any case, I don't think this is the pr to address VALUES optimisations
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r760717697
##########
File path: core/src/main/java/org/apache/calcite/adapter/jdbc/JdbcRules.java
##########
@@ -332,6 +332,9 @@ protected JdbcJoinRule(Config config) {
* @return Whether condition is supported
*/
private static boolean canJoinOnCondition(RexNode node) {
+ if (node.isAlwaysTrue() || node.isAlwaysFalse()) {
+ return true;
+ }
Review comment:
i agree. with a comment that this would be TRUE or FALSE.
--
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
[GitHub] [calcite] julianhyde commented on a change in pull request #2620: [CALCITE-4907] JdbcJoin does not convert Join with always true/false …
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #2620:
URL: https://github.com/apache/calcite/pull/2620#discussion_r760717357
##########
File path: core/src/test/java/org/apache/calcite/test/JdbcAdapterTest.java
##########
@@ -50,23 +50,24 @@
* same time. */
private static final ReentrantLock LOCK = new ReentrantLock();
- /** VALUES is not pushed down, currently. */
@Test void testValuesPlan() {
Review comment:
update the comment; don't remove 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