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/11/17 04:38:30 UTC
[GitHub] [calcite] hqx871 opened a new pull request #2266: [CALCITE-4402] The equalsDeep returns wrong result
hqx871 opened a new pull request #2266:
URL: https://github.com/apache/calcite/pull/2266
The method equalsDeep of SqlCall will returns wrong result when used to compare count and count_distinct, because it does not compare the functionQuantifier
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525112906
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlCall.java
##########
@@ -146,6 +146,9 @@ public int operandCount() {
return litmus.fail("{} != {}", this, node);
}
SqlCall that = (SqlCall) node;
+ if (!equalDeep(this.getFunctionQuantifier(), that.getFunctionQuantifier(), litmus)) {
+ return litmus.fail("{} != {}", this, node);
Review comment:
```suggestion
return litmus.fail("{} != {} (functionQualifier differs)", this, node);
```
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525092764
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlEqualsDeepTest.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.test;
+
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.util.Litmus;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Unit test for fix [CALCITE-4402] equalsDeep.
+ */
+class SqlEqualsDeepTest {
+
+ @Test void testCountEqualsDeep() throws SqlParseException {
+ assertEqualsDeep("count(a)", "count(a)", true);
+ assertEqualsDeep("count(distinct a)", "count(distinct a)", true);
+ assertEqualsDeep("count(distinct a)", "count(a)", false);
+ }
+
+ private void assertEqualsDeep(String expr0, String expr1, boolean expected)
+ throws SqlParseException {
+
+ SqlNode sqlNode0 = parseExpression(expr0);
+ SqlNode sqlNode1 = parseExpression(expr1);
+ boolean actual = sqlNode0.equalsDeep(sqlNode1, Litmus.IGNORE);
+ String reason = expr0 + " equals " + expr1;
+
+ assertEquals(expected, actual, reason);
Review comment:
```suggestion
assertEquals(expected, sqlNode0.equalsDeep(sqlNode1, Litmus.IGNORE), () -> expr0 + " equalsDeep " + expr1);
```
----------------------------------------------------------------
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] vlsi commented on a change in pull request #2266: [CALCITE-4402] The equalsDeep returns wrong result
Posted by GitBox <gi...@apache.org>.
vlsi commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525056951
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlEqualsDeepTest.java
##########
@@ -0,0 +1,49 @@
+/*
+ * 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.sql.test;
+
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.util.Litmus;
+
+import org.junit.jupiter.api.Test;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+
+/**
+ * Unit test for fix [CALCITE-4402] The equalsDeep returns wrong result.
+ */
+class SqlEqualsDeepTest {
+
+ @Test void testCountEqualsDeep() throws SqlParseException {
+
+ SqlNode countExpression = parseExpression("count(a)");
+ SqlNode countDistinctExpression = parseExpression("count(distinct a)");
+
+ assertThat("count(a) doesn't equals count(a)",
+ countExpression.equalsDeep(parseExpression("count(a)"), Litmus.IGNORE));
Review comment:
Please factor this to a method with two expression arguments.
The method should be like `assertThat("{" + expr1 + "}.equalsDeep({" + expr2 + "})", parseExpression(expr1).equals(parseExpression(expr2)), 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hqx871 commented on a change in pull request #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
hqx871 commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525274593
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlEqualsDeepTest.java
##########
@@ -0,0 +1,53 @@
+/*
+ * 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.sql.test;
+
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.util.Litmus;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
+ * Unit test for fix [CALCITE-4402] equalsDeep.
+ */
+class SqlEqualsDeepTest {
+
+ @Test void testCountEqualsDeep() throws SqlParseException {
+ assertEqualsDeep("count(a)", "count(a)", true);
+ assertEqualsDeep("count(distinct a)", "count(distinct a)", true);
+ assertEqualsDeep("count(distinct a)", "count(a)", false);
+ }
+
+ private void assertEqualsDeep(String expr0, String expr1, boolean expected)
+ throws SqlParseException {
+
+ SqlNode sqlNode0 = parseExpression(expr0);
+ SqlNode sqlNode1 = parseExpression(expr1);
+ boolean actual = sqlNode0.equalsDeep(sqlNode1, Litmus.IGNORE);
+ String reason = expr0 + " equals " + expr1;
+
+ assertEquals(expected, actual, reason);
Review comment:
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] hqx871 commented on a change in pull request #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
hqx871 commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525926724
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlEqualsDeepTest.java
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.sql.test;
+
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.util.Litmus;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
Review comment:
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] rubenada merged pull request #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
rubenada merged pull request #2266:
URL: https://github.com/apache/calcite/pull/2266
----------------------------------------------------------------
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 #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525907595
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlCall.java
##########
@@ -146,6 +146,9 @@ public int operandCount() {
return litmus.fail("{} != {}", this, node);
}
SqlCall that = (SqlCall) node;
+ if (!equalDeep(this.getFunctionQuantifier(), that.getFunctionQuantifier(), litmus)) {
+ return litmus.fail("{} != {} (functionQualifier differs)", this, node);
Review comment:
minor: message says "functionQualifier", and I guess it should be "function quantifier".
Also, personally I would prefer to add this new check after the operator's name check (i.e. reach the verification of function quantifier only in those cases where operator names' match).
----------------------------------------------------------------
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 #2266: [CALCITE-4402] The equalsDeep returns wrong result
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#issuecomment-728860555
IMHO the commit message (and Jira title) could be more specific, e.g. "SqlCall#equalsDeep does not take into account the function quantifier"
----------------------------------------------------------------
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 #2266: [CALCITE-4402] The equalsDeep returns wrong result
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#issuecomment-728775106
Would it be possible to have a unit test?
----------------------------------------------------------------
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 #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525986104
##########
File path: core/src/main/java/org/apache/calcite/sql/SqlCall.java
##########
@@ -153,6 +153,9 @@ public int operandCount() {
if (!this.getOperator().getName().equalsIgnoreCase(that.getOperator().getName())) {
return litmus.fail("{} != {}", this, node);
}
+ if (!equalDeep(this.getFunctionQuantifier(), that.getFunctionQuantifier(), litmus)) {
+ return litmus.fail("{} != {} (function qualifier differs)", this, node);
Review comment:
Sorry, the message still uses an incorrect term :
"function **qualifier** differs"
it should be
"function **quantifier** differs"
Also, please add you name between parenthesis at the end of the commit message, as per https://calcite.apache.org/develop/#contributing _"If you are not a committer, add your name in parentheses at the end of the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rubenada commented on a change in pull request #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
rubenada commented on a change in pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#discussion_r525910302
##########
File path: core/src/test/java/org/apache/calcite/sql/test/SqlEqualsDeepTest.java
##########
@@ -0,0 +1,52 @@
+/*
+ * 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.sql.test;
+
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.parser.SqlParseException;
+import org.apache.calcite.sql.parser.SqlParser;
+import org.apache.calcite.util.Litmus;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+/**
Review comment:
minor: usually javadoc for test cases related to a certain Jira follow the following format:
```
/** Test case for
* <a href="https://issues.apache.org/jira/browse/CALCITE-4402">[CALCITE-4402]
* SqlCall#equalsDeep does not take into account the function quantifier</a>. */
```
----------------------------------------------------------------
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 #2266: [CALCITE-4402] SqlCall#equalsDeep does not take into account the function quantifier
Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2266:
URL: https://github.com/apache/calcite/pull/2266#issuecomment-729634370
LGTM
----------------------------------------------------------------
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