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/01/10 10:21:16 UTC

[GitHub] [calcite] Aaaaaaron opened a new pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Aaaaaaron opened a new pull request #2320:
URL: https://github.com/apache/calcite/pull/2320


   …


----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2320:
URL: https://github.com/apache/calcite/pull/2320#discussion_r568417115



##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -122,7 +122,7 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
       final SqlCall operand0 = ((SqlCall) query).operand(0);
       final SqlCall operand1 = ((SqlCall) query).operand(1);
       final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
-          && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+          || rowTypeCoercion(scope, operand1, columnIndex, targetType);
       // Update the nested SET operator node type.

Review comment:
       @amaliujia didn't reproduce the case in that ut, will take a look later, 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 commented on pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Posted by GitBox <gi...@apache.org>.
rubenada commented on pull request #2320:
URL: https://github.com/apache/calcite/pull/2320#issuecomment-757456022


   Would it be possible to have a unit test "showing" the bug?


----------------------------------------------------------------
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] danny0405 commented on a change in pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #2320:
URL: https://github.com/apache/calcite/pull/2320#discussion_r554666740



##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -122,7 +122,7 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
       final SqlCall operand0 = ((SqlCall) query).operand(0);
       final SqlCall operand1 = ((SqlCall) query).operand(1);
       final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
-          && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+          || rowTypeCoercion(scope, operand1, columnIndex, targetType);
       // Update the nested SET operator node type.

Review comment:
       Thanks, instead of `||` or `&&` to chain each union branch type coercion, we should use a `for-loop` to ensure each branch are checked.
   
   You can add a test case in the `TypeCoercionTest#testSetOperations`.




----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2320:
URL: https://github.com/apache/calcite/pull/2320#discussion_r568417115



##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -122,7 +122,7 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
       final SqlCall operand0 = ((SqlCall) query).operand(0);
       final SqlCall operand1 = ((SqlCall) query).operand(1);
       final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
-          && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+          || rowTypeCoercion(scope, operand1, columnIndex, targetType);
       // Update the nested SET operator node type.

Review comment:
       @amaliujia didn't reproduce the case in that ut, will take a look later, 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] Aaaaaaron commented on pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on pull request #2320:
URL: https://github.com/apache/calcite/pull/2320#issuecomment-757463307


   > Would it be possible to have a unit test "showing" the bug?
   
   Hi rubenada, sorry for the ut, this part is pretty hard for ut cover, see this: https://github.com/apache/calcite/pull/2056


----------------------------------------------------------------
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] amaliujia commented on a change in pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Posted by GitBox <gi...@apache.org>.
amaliujia commented on a change in pull request #2320:
URL: https://github.com/apache/calcite/pull/2320#discussion_r566366088



##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -122,7 +122,7 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
       final SqlCall operand0 = ((SqlCall) query).operand(0);
       final SqlCall operand1 = ((SqlCall) query).operand(1);
       final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
-          && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+          || rowTypeCoercion(scope, operand1, columnIndex, targetType);
       // Update the nested SET operator node type.

Review comment:
       @Aaaaaaron  any update on the test in `TypeCoercionTest#testSetOperations`?




----------------------------------------------------------------
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] Aaaaaaron commented on a change in pull request #2320: [CALCITE-4458] The "rowTypeCoercion" for "SET" operations should be "OR" relationship (Jiatao Tao)

Posted by GitBox <gi...@apache.org>.
Aaaaaaron commented on a change in pull request #2320:
URL: https://github.com/apache/calcite/pull/2320#discussion_r554895322



##########
File path: core/src/main/java/org/apache/calcite/sql/validate/implicit/TypeCoercionImpl.java
##########
@@ -122,7 +122,7 @@ public TypeCoercionImpl(RelDataTypeFactory typeFactory, SqlValidator validator)
       final SqlCall operand0 = ((SqlCall) query).operand(0);
       final SqlCall operand1 = ((SqlCall) query).operand(1);
       final boolean coerced = rowTypeCoercion(scope, operand0, columnIndex, targetType)
-          && rowTypeCoercion(scope, operand1, columnIndex, targetType);
+          || rowTypeCoercion(scope, operand1, columnIndex, targetType);
       // Update the nested SET operator node type.

Review comment:
       @danny0405 Thanks, Danny!




----------------------------------------------------------------
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