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/05/11 00:31:26 UTC
[GitHub] [calcite] talatuyarer opened a new pull request #1971: [CALCITE-3627] New Row Null Policy
talatuyarer opened a new pull request #1971:
URL: https://github.com/apache/calcite/pull/1971
As describe under CALCITE-3627 ticket. Current row policy is wrong. When ROW has a null column. Calcite returns null for ROW value. I am working on Apache Beam SQL Row support. Because of ANY policy RexToLixTranslator generate wrong code. This is my first pr for calcite project. Thanks for your comments in advance.
----------------------------------------------------------------
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] DonnyZone commented on a change in pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1971:
URL: https://github.com/apache/calcite/pull/1971#discussion_r422758224
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/NullPolicy.java
##########
@@ -25,6 +25,10 @@
* ANY whenever possible.</p>
*/
public enum NullPolicy {
+
+ /** Returns null if and only if all of the arguments are null;
+ * If all of the arguments are false return false otherwise true. */
Review comment:
> If all of the arguments are false return false otherwise true.
Do we support this? I think the policy is just for `null` value.
----------------------------------------------------------------
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] talatuyarer closed pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
talatuyarer closed pull request #1971:
URL: https://github.com/apache/calcite/pull/1971
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #1971:
URL: https://github.com/apache/calcite/pull/1971#issuecomment-626452092
Hi @talatuyarer, could you please update the commit message as the format described in [document](https://calcite.apache.org/develop/)?
----------------------------------------------------------------
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] talatuyarer commented on pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
talatuyarer commented on pull request #1971:
URL: https://github.com/apache/calcite/pull/1971#issuecomment-1002896694
I updated my pr but looks like it does not take updated version. I will create new pr for this
--
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-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] DonnyZone commented on a change in pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1971:
URL: https://github.com/apache/calcite/pull/1971#discussion_r422758428
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -1146,7 +1147,10 @@ private static Expression implementNullSemantics(
list.add(
translator.translate(
operand.e, NullAs.IS_NULL));
- translator = translator.setNullable(operand.e, false);
+ //If column is not ROW
Review comment:
Whitspace after `//`
----------------------------------------------------------------
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] DonnyZone commented on a change in pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1971:
URL: https://github.com/apache/calcite/pull/1971#discussion_r422759894
##########
File path: core/src/main/java/org/apache/calcite/adapter/enumerable/RexImpTable.java
##########
@@ -1166,9 +1170,22 @@ private static Expression implementNullSemantics(
default:
throw new AssertionError();
}
+
+ // Condition for regular cases: v0 == null || v1 == null
+ Expression condition = Expressions.foldOr(list);
+ if (nullPolicy == NullPolicy.ALL) {
+ // if some operands are not nullable ROW can not be null
Review comment:
To unify the comment style, capitalize `if` as `If`.
----------------------------------------------------------------
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] DonnyZone commented on a change in pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on a change in pull request #1971:
URL: https://github.com/apache/calcite/pull/1971#discussion_r422759509
##########
File path: core/src/test/resources/sql/struct.iq
##########
@@ -83,4 +83,81 @@ where t.struct = ROW(2, ROW(3,4));
!ok
+# [CALCITE-3627] Null check if all fields of ROW are null
Review comment:
Can we combine these tests into one?
```
select ROW(...),
ROW(...), ... ,
ROW(...)
```
----------------------------------------------------------------
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] DonnyZone commented on pull request #1971: [CALCITE-3627] New Row Null Policy
Posted by GitBox <gi...@apache.org>.
DonnyZone commented on pull request #1971:
URL: https://github.com/apache/calcite/pull/1971#issuecomment-626447764
Thanks for pushing this work forward. I ever tried (but failed) to find some similar functions in other DB products. This makes me confused about whether we should treat `[null, null, .., null]` as `null`.
That is, to introduce `NullPolicy.ALL` or use `NullPolicy.NONE`? At least, current `NullPolicy.ANY` is wrong.
Personally, I'm inclined to introduce `NullPolicy.ALL`, as it follows SQL standard.
Also cc @rubenada @zabetak
----------------------------------------------------------------
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