[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1045286809 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: @libenchao I reverted my changes for "multiMap" and did rebase -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1044438897 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: Yes, the problem does not exists for "multiMap" at the moment. But this change avoids possible problem. Although I started to think, that I should revert changes for "multiMap" and create separate ticket for improving optimizer. It is quite bad idea to reuse mutable collections at all: https://github.com/dssysolyatin/calcite/commit/626a5f48ef9e69e543aeec277a4f38000a190b10 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1044438897 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: Yes, the problem does not exists for "multiMap" at the moment. But this change avoids possible problem. Although I started to think, that I should revert changes for "multiMap" and create separate ticket for improving optimizer. It is quite bad idea to reuse mutable collections at all: https://github.com/apache/calcite/commit/a7bedd6595ebe69bd93e9fcc3f05850da5f673b2 -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1038245590 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: @rubenada For case when tempList is used, the query returns empty result without this fix. The following test case covers it: ``` with CTE1(rownr1, val1) as ( select ROW_NUMBER() OVER(ORDER BY id ASC), id from (values (1), (2)) as Vals1(id) ), CTE2(rownr2, val2) as ( select ROW_NUMBER() OVER(ORDER BY id ASC), id from (values (1), (2)) as Vals2(id) ) select CTE1.rownr1, CTE1.val1, CTE2.rownr2, CTE2.val2 from CTE1,CTE2 where CTE1.val1 = CTE2.val2; ++--++--+ | ROWNR1 | VAL1 | ROWNR2 | VAL2 | ++--++--+ | 1 |1 | 1 |1 | | 2 |2 | 2 |2 | ++--++--+ (2 rows) ``` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1038206955 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: @libenchao @rubenada As I wrote above "multiMap" case works without this fix. But it works only because expressions which create and fill multiMap are different. I set optimize flag = false for "multiMap" only for safety, because it is really easy to make a mistake here. I am not against optimizations, but in my opinion, this optimization does more harm than good. ``` Pseudo code for "multiMap" case <-- first EnumerableWindow --> multiMap = new SortedMultiMap() <-- luckely optimizer caches only this expression fillMultimap(multiMap) multiMap.clear() <-- second EnumerableWindow --> fillMap(multiMap) multiMap.clear() ``` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977: URL: https://github.com/apache/calcite/pull/2977#discussion_r1038206955 ## core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java: ## @@ -693,7 +696,7 @@ private static Pair getPartitionIterator( } Expression multiMap_ = builder.append( -"multiMap", Expressions.new_(SortedMultiMap.class)); +"multiMap", Expressions.new_(SortedMultiMap.class), false); Review Comment: @libenchao @rubenada As I wrote above "multiMap" case works without this fix. But it works only because expressions which create and fill multiMap are different. I set optimize flag = true for "multiMap" only for safety, because it is really easy to make a mistake here. I am not against optimizations, but in my opinion, this optimization does more harm than good. ``` Pseudo code for "multiMap" case <-- first EnumerableWindow --> multiMap = new SortedMultiMap() <-- luckely optimizer caches only this expression fillMultimap(multiMap) multiMap.clear() <-- second EnumerableWindow --> fillMap(multiMap) multiMap.clear() ``` -- 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-unsubscr...@calcite.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977:
URL: https://github.com/apache/calcite/pull/2977#discussion_r1033016387
##
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java:
##
@@ -693,7 +696,7 @@ private static Pair
getPartitionIterator(
}
Expression multiMap_ =
builder.append(
-"multiMap", Expressions.new_(SortedMultiMap.class));
+"multiMap", Expressions.new_(SortedMultiMap.class), false);
Review Comment:
multiMap works OK at the moment even with optimize = true. But it is very
unsafe. I will try to explain why. The different between multiMap and tempList
cases are following.
Code for tempList case looks like:
```
final java.util.List tempList = (java.util.List)
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[] {1,2})
.into(new java.util.ArrayList()); <--this expression will be
reused
tempList.clear()
<-- Second EnumerableWindow will use the same tempList and will not fill
tempList again -->
```
Code for multiMap looks like:
```
final org.apache.calcite.runtime.SortedMultiMap multiMap =
new org.apache.calcite.runtime.SortedMultiMap(); <--this expression
will be reused
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[]
{1,2}).foreach()
multiMap.clear()
<-- Second EnumerableWindow will use the same multiMap but also will refill
it -->
```
Code works OK for multiMap only because the creation and filling of the
multiMap are in different expressions. But it is very unsafe. If for some
reason creating and filling multiMap are in the same expression code will also
stop working.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977:
URL: https://github.com/apache/calcite/pull/2977#discussion_r1033016387
##
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java:
##
@@ -693,7 +696,7 @@ private static Pair
getPartitionIterator(
}
Expression multiMap_ =
builder.append(
-"multiMap", Expressions.new_(SortedMultiMap.class));
+"multiMap", Expressions.new_(SortedMultiMap.class), false);
Review Comment:
multiMap works OK at the moment even with optimize = true. But it is very
unsafe. I will try to explain why. The different between multiMap and tempList
cases are following.
Code for tempList case looks like:
```
final java.util.List tempList = (java.util.List)
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[] {1,2})
.into(new java.util.ArrayList()); <--this expression will be
reused
tempList.clear()
<-- Second EnumerableWindow will use the same tempList and will not fill
tempList again -->
```
Code for multiMap looks like:
```
final org.apache.calcite.runtime.SortedMultiMap multiMap =
new org.apache.calcite.runtime.SortedMultiMap(); <--this expression
will be reused
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[]
{1,2}).foreach()
multiMap.clear()
<-- Second EnumerableWindow will refill multiMap -->
```
Code works OK for multiMap only because the creation and filling of the
multiMap are in different expressions. But it is very unsafe. If for some
reason creating and filling multiMap are in the same expression code will also
stop working.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [calcite] dssysolyatin commented on a diff in pull request #2977: [CALCITE-5388] No result when using ROW_NUMBER in two common table expressions
dssysolyatin commented on code in PR #2977:
URL: https://github.com/apache/calcite/pull/2977#discussion_r1033016387
##
core/src/main/java/org/apache/calcite/adapter/enumerable/EnumerableWindow.java:
##
@@ -693,7 +696,7 @@ private static Pair
getPartitionIterator(
}
Expression multiMap_ =
builder.append(
-"multiMap", Expressions.new_(SortedMultiMap.class));
+"multiMap", Expressions.new_(SortedMultiMap.class), false);
Review Comment:
multiMap works OK at the moment even with optimize = true. But it is very
unsafe. I will try to explain why. The different between multiMap and tempList
cases are following.
Code for tempList case looks like:
```
final java.util.List tempList = (java.util.List)
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[] {1,2})
.into(new
java.util.ArrayList()); <-- this expression will be reused.
tempList.clear()
<> Second EnumerableWindow will use the same tempList and will not fill
tempList again
```
Code for multiMap looks like:
```
final org.apache.calcite.runtime.SortedMultiMap multiMap = new
org.apache.calcite.runtime.SortedMultiMap(); <--this expression will be reused
org.apache.calcite.linq4j.Linq4j.asEnumerable(new Integer[]
{1,2}).foreach()
multiMap.clear()
<> Second EnumerableWindow will refill
```
Code works OK for multiMap only because the creation and filling of the
multiMap are in different expressions. But it is very unsafe. If for some
reason creating and filling multiMap are in the same expression code will also
stop working.
--
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-unsubscr...@calcite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
