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 2019/12/24 09:43:35 UTC

[GitHub] [calcite] DonnyZone opened a new pull request #1684: [CALCITE-3627] Incorrect null semantic for ROW function

DonnyZone opened a new pull request #1684: [CALCITE-3627] Incorrect null semantic for ROW function
URL: https://github.com/apache/calcite/pull/1684
 
 
   As illustrated in [CALCITE-3627](https://issues.apache.org/jira/browse/CALCITE-3627)
   This PR:
   (1) allows `ROW` function's return type can be nullable;
   (2) introduces a new NullPolicy (`ALL`) to implements the special semantic.

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


With regards,
Apache Git Services

[GitHub] [calcite] DonnyZone commented on issue #1684: [CALCITE-3627] Incorrect null semantic for ROW function

Posted by GitBox <gi...@apache.org>.
DonnyZone commented on issue #1684: [CALCITE-3627] Incorrect null semantic for ROW function
URL: https://github.com/apache/calcite/pull/1684#issuecomment-570981413
 
 
   Thanks for your feedback! @rubenada 

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


With regards,
Apache Git Services

[GitHub] [calcite] rubenada commented on issue #1684: [CALCITE-3627] Incorrect null semantic for ROW function

Posted by GitBox <gi...@apache.org>.
rubenada commented on issue #1684: [CALCITE-3627] Incorrect null semantic for ROW function
URL: https://github.com/apache/calcite/pull/1684#issuecomment-570529739
 
 
   @DonnyZone I think you are in the right track when you say that the nullability of `ROW` function is not the same as the nullability of a composite type; perhaps @zabetak could confirm, I think he knows the SQL standard better than myself :)
   
   Apart from that, I agree with you, `NullPolicy` for `ROW` function should not be ANY, so the current patch LGTM.
   If possible, I would add more tests (maybe in struct.iq?) to verify the different ROW nullability scenarios, e.g. (expected values taken from pg):
   ```
   select row(null, null, null) is null -- expected true
   select row(null, null, null) is not null -- expected false
   
   select row(null, 1, null) is null -- expected false
   select row(null, 1, null) is not null -- expected false
   select not( row(null, 1, null) is null ) -- expected true
   ```

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


With regards,
Apache Git Services

[GitHub] [calcite] DonnyZone commented on issue #1684: [CALCITE-3627] Incorrect null semantic for ROW function

Posted by GitBox <gi...@apache.org>.
DonnyZone commented on issue #1684: [CALCITE-3627] Incorrect null semantic for ROW function
URL: https://github.com/apache/calcite/pull/1684#issuecomment-570499622
 
 
   Maybe we need more discussion on the case of all `null` elements for `ARRAY/ROW` function.
   ```
   [null, null, null] is null
   ```
   I investigated the query below in pg:
   ```
   select array[null, null, null] is null
   ```
   It returns `false`.
   
   The function's nullability seems to be a little different with that of row's struct type.
   What I am sure is the `NullPolicy` for `ROW` function should not be `ANY`.
   
   Hi, @zabetak @rubenada, I saw your in-depth discussion in [CALCITE-2464](https://issues.apache.org/jira/browse/CALCITE-2464),  could you help to confirm 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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [calcite] danny0405 commented on a change in pull request #1684: [CALCITE-3627] Incorrect null semantic for ROW function

Posted by GitBox <gi...@apache.org>.
danny0405 commented on a change in pull request #1684: [CALCITE-3627] Incorrect null semantic for ROW function
URL: https://github.com/apache/calcite/pull/1684#discussion_r362185314
 
 

 ##########
 File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
 ##########
 @@ -1111,9 +1114,15 @@ private static Expression implementNullSemantics(
       default:
         throw new AssertionError();
       }
+      final Expression condition =
+          nullPolicy == NullPolicy.ALL
+          ? (list.size() < conditionalOps.size()
+              ? FALSE_EXPR
 
 Review comment:
   Replace with if else

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


With regards,
Apache Git Services