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 2020/06/17 15:16:09 UTC
[GitHub] [calcite] rubenada opened a new pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
rubenada opened a new pull request #2031:
URL: https://github.com/apache/calcite/pull/2031
Jira: https://issues.apache.org/jira/browse/CALCITE-4063
There is a mismatch regarding what an UNNEST of a "simple" ROW (just 1 item) is expected to return as element type (integer), and what its enumerable actually returns (List).
Solves the issue by using a special lambda for selectMany in these situations.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada edited a comment on pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
rubenada edited a comment on pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#issuecomment-646728681
Another advantage for `EnumerableUncollectTest` is that I could add a test, that is currently disabled due to another bug `@Disabled("CALCITE-4064")`. I'm not sure we can do such a thing easily in `struct.iq`.
Having said that, I am open for removing either of them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#issuecomment-646687829
@michaelmior you are right, they are the same tests in two different places.
We could remove one of them (I guess struct.iq?).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#issuecomment-647020791
@michaelmior , changes from `struct.iq` removed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#discussion_r442893520
##########
File path: core/src/test/java/org/apache/calcite/test/enumerable/EnumerableUncollectTest.java
##########
@@ -0,0 +1,189 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.test.enumerable;
+
+import org.apache.calcite.config.CalciteConnectionProperty;
+import org.apache.calcite.config.Lex;
+import org.apache.calcite.test.CalciteAssert;
+
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+
+class EnumerableUncollectTest {
+
+ @Test void simpleUnnestArray() {
+ final String sql = "select * from UNNEST(array[3, 4]) as T2(y)";
+ tester()
+ .query(sql)
+ .returnsUnordered(
Review comment:
If I am not mistaken, our UNNEST implementation keeps the order, however SQL standard says that UNNEST does not guarantee to keep order, that is why the "WITH ORDINALITY" exists. This the reason why `returnsUnordered` is used here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior merged pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
michaelmior merged pull request #2031:
URL: https://github.com/apache/calcite/pull/2031
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior commented on a change in pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
michaelmior commented on a change in pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#discussion_r442889550
##########
File path: core/src/test/java/org/apache/calcite/test/enumerable/EnumerableUncollectTest.java
##########
@@ -0,0 +1,189 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.calcite.test.enumerable;
+
+import org.apache.calcite.config.CalciteConnectionProperty;
+import org.apache.calcite.config.Lex;
+import org.apache.calcite.test.CalciteAssert;
+
+import org.junit.jupiter.api.Disabled;
+import org.junit.jupiter.api.Test;
+
+class EnumerableUncollectTest {
+
+ @Test void simpleUnnestArray() {
+ final String sql = "select * from UNNEST(array[3, 4]) as T2(y)";
+ tester()
+ .query(sql)
+ .returnsUnordered(
Review comment:
Should these all really be unordered? I'm not sure what other implementations do, but I would have expected array elements to remain in order (same for the tests below).
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior commented on pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
michaelmior commented on pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#issuecomment-646726162
I would say `struct.iq` is much simpler so I would normally vote for keeping that although it doesn't capture the order invariance property you mentioned.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior commented on pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
michaelmior commented on pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#issuecomment-646813897
Given that you've already gone to the effort to write `EnumerableUncollectTest`, I'd say keep that one and remove `struct.iq`. Thanks!
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] michaelmior commented on pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
michaelmior commented on pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#issuecomment-646684247
What is the difference between `EnumerableUncollectTest` and the new tests in `struct.iq`? They seem to cover the same cases.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on pull request #2031: [CALCITE-4063] Unnest an array of single-item structs causes ClassCastException
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2031:
URL: https://github.com/apache/calcite/pull/2031#issuecomment-646728681
Another advantage for `EnumerableUncollectTest` is that I could add a test, that is currently disabled due to another bug `@Disabled("CALCITE-3660")`. I'm not sure we can do such a thing easily in `struct.iq`.
Having said that, I am open for removing either of them.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org