[ https://issues.apache.org/jira/browse/CALCITE-4085?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17144724#comment-17144724 ]
Dawid Wysakowicz edited comment on CALCITE-4085 at 6/25/20, 7:48 AM: --------------------------------------------------------------------- I can't see how the [1] can be of any help in my use case. It's just a helper method around {{RelDataTypeFactory#createTypeWithNullability}}. I don't have any problems with making a record type nullable. The problem is with preserving the nullability of nested fields. Because {{RelDataTypeFactory#createTypeWithNullability}} changes the nested types. Calling [1] on a type {{USER(id NOT NULL) NOT NULL}} will change it to {{USER(id <NULLABLE>) <NULLABLE>}}. As I pointed out in my email I have a way to workaround that in the Flink code. What I can't do in Flink in a reasonable way is to keep the _"[A].[B]", if [A] is nullable then [A].[B] should also be nullable_ logic. {quote}I still think the nullability propagation in type factory is correct. Based on the SQL standard and the fact that: for expression "[A].[B]", if [A] is nullable then [A].[B] should also be nullable. {quote} On this topic I agree we disagree ;) Personally I think this is the characteristic of the operator (SqlDotOperator, SqlItemOperator, RexFieldAccess) not the type itself. The same way as it is property of the {{AND}} operator that it returns nullable type if any of the operands is nullable. You do not fix the {{BOOLEAN}} type in typeFactory to be always nullable, because of the {{AND}} operator. Nevertheless because as I said there is a way to override that behaviour in our type factory in a reasonable way, I don't want to insist on changing that behaviour in Calcite. What I'd really appreciate is to have the logic in the operators as well, as they can not be overriden in a good way (without copying over the classes). Yes, I agree with your analysis of the {{TableFunctionTest#testTableFunction}} but for me it shows that either: * the mapping from POJO to JavaType is wrong * the logic in [2] is wrong If you think adding an exclusion for the {{JavaType}} is a right solution there, I can do that. I will update the PR shortly. Personally though I don't like it as it looks like a hack for me. I don't necessarily understand how are the {{JavaTypes}} different in this case than regular sql types. Lastly, I'd like to ask again about the changes in {{RexBuilder#makeFieldAccessInternal}} and {{SqlValidatorImpl.DeriveTypeVisitor#visit(SqlIdentifier)}}. Those are equivalents of {{SqlDotOperator}} when we are resolving a field access like: {code:java} CREATE TYPE User ( id INT NOT NULL ); CREATE TABLE Accounts ( user USER -- nullable ) SELECT t.user.id FROM Accounts t; {code} What would be your suggestion how can I integrate the same fix as for {{SqlDotOperator}} and {{SqlItemOperator}} in this case? As mentioned before in this ticket and also in my email I can override that in an acceptable way in our codebase, but would also appreciate a built-in support for it. Any suggestions on that? [1] [https://github.com/apache/calcite/blob/0769a8b31cbbeb5bca66ade30cf3710523da4aaa/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java#L518] [2] [https://github.com/apache/calcite/blob/0769a8b31cbbeb5bca66ade30cf3710523da4aaa/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java#L338] was (Author: dawidwys): I can't see how the [1] can be of any help in my use case. It's just a helper method around {{RelDataTypeFactory#createTypeWithNullability}}. I don't have any problems with making a record type nullable. The problem is with preserving the nullability of nested fields. Because {{RelDataTypeFactory#createTypeWithNullability}} changes the nested types. Calling [1] on a type {{USER(id NOT NULL) NOT NULL}} will change it to {{USER(id <NULLABLE>) <NULLABLE>}}. As I pointed out in my email I have a way to workaround that in the Flink code. What I can't do in Flink in a reasonable way is to keep the _"[A].[B]", if [A] is nullable then [A].[B] should also be nullable_ logic. ??I still think the nullability propagation in type factory is correct. Based on the SQL standard and the fact that: for expression "[A].[B]", if [A] is nullable then [A].[B] should also be nullable.?? On this topic I agree we disagree ;) Personally I think this is the characteristic of the operator (SqlDotOperator, SqlItemOperator, RexFieldAccess) not the type itself. The same way as it is property of the {{AND}} operator that it returns nullable type if any of the operands is nullable. You do not fix the {{BOOLEAN}} type in typeFactory to be always nullable, because of the {{AND}} operator. Nevertheless because as I said there is a way to override that behaviour in our type factory in a reasonable way, I don't want to insist on changing that behaviour in Calcite. What I'd really appreciate is to have the logic in the operators as well, as they can not be overriden in a good way (without copying over the classes). Yes, I agree with your analysis of the {{TableFunctionTest#testTableFunction}} but for me it shows that either: * the mapping from POJO to JavaType is wrong * the logic in [2] is wrong If you think adding an exclusion for the {{JavaType}} is a right solution there, I can do that. I will update the PR shortly. Personally though I don't like it as it looks like a hack for me. I don't necessarily understand how are the {{JavaTypes}} different in this case than regular sql types. Lastly, I'd like to ask again about the changes in {{RexBuilder#makeFieldAccessInternal}} and {{SqlValidatorImpl.DeriveTypeVisitor#visit(SqlIdentifier)}}. Those are equivalents of {{SqlDotOperator}} when we are resolving a field access like: {code} CREATE TYPE User ( id INT NOT NULL ); CREATE TABLE Accounts ( user USER -- nullable ) SELECT t.user.id FROM Accounts t; {code} What would be your suggestion how can I integrate the same fix as for {{SqlDotOperator}} and {{SqlItemOperator}} in this case? As mentioned before in this ticket and also in my email I can override that in an acceptable way in our codebase, but would also appreciate a built-in support for it. Any suggestions on that? [1] https://github.com/apache/calcite/blob/0769a8b31cbbeb5bca66ade30cf3710523da4aaa/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactory.java#L518 [2] https://github.com/apache/calcite/blob/0769a8b31cbbeb5bca66ade30cf3710523da4aaa/core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java#L338 > Improve nullability support for fields of structured type > --------------------------------------------------------- > > Key: CALCITE-4085 > URL: https://issues.apache.org/jira/browse/CALCITE-4085 > Project: Calcite > Issue Type: Improvement > Components: core > Reporter: Dawid Wysakowicz > Assignee: Dawid Wysakowicz > Priority: Major > Time Spent: 10m > Remaining Estimate: 0h > > As discussed in > https://lists.apache.org/thread.html/r602ac95fff23dd1ef974fb396df7be061ab861384ec42f5c57ce0bc2%40%3Cdev.calcite.apache.org%3E > I would like to change the way a type of a field of a record is derived at > couple of locations. This helps frameworks such as Apache Flink to build > support for nullable records with not null fields. > I suggest to change: > * SqlDotOperator#deriveType > * SqlItemOperator#inferReturnType > * AliasNamespace#validateImpl > * RexBuilder#makeFieldAccessInternal > * SqlValidatorImpl.DeriveTypeVisitor#visit(SqlIdentifier) -- This message was sent by Atlassian Jira (v8.3.4#803005)