[jira] [Comment Edited] (CALCITE-2464) Allow to set nullability for columns of structured types

["Ruben Quesada Lopez (JIRA)" <ji...@apache.org>]

    [ https://issues.apache.org/jira/browse/CALCITE-2464?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16743210#comment-16743210 ] 

Ruben Quesada Lopez edited comment on CALCITE-2464 at 1/15/19 4:46 PM:
-----------------------------------------------------------------------

[~julianhyde], [~zabetak], thanks for your comments.
My initial ([PR#898|https://github.com/apache/calcite/pull/898/]) follows a "conservative approach": struct types behave as before (they have by default isNullable=false, i.e. NOT NULL constraint), but we can now create struct types with isNullable=true via {{createTypeWithNullability(recordType, true)}} method (which so far did not change the nullability of the struct type itself). This means that, in order to have a nullable struct type, we'd need to go through createTypeWithNullability, either explicitly or implicitly (e.g. during a join result type computation).

However, the default behavior does not change, which means that the code proposed by [~zabetak] in the comment above (or any similar test added to type.iq) will still fail. In order to make it work, I think we would need to change the default behavior, and start creating nullable struct types from the beginning (perhaps the new default should be isNullable=true?), for example in JavaTypeFactoryImpl.java:
{code}
public RelDataType createType(Type type) {
  ...
  // return createStructType(clazz); // current code
  return createTypeWithNullability(createStructType(clazz), true); // new code?
  ...
}
{code}
But this change would introduce some side effects (many unit tests fail, maybe backwards compatibility might be an issue?). I am willing to carry on the task and fix the tests that will need to be modified.


was (Author: rubenql):
[~julianhyde], [~zabetak], thanks for your comments.
My initial ([PR#898|https://github.com/apache/calcite/pull/898/]) follows a "conservative approach": struct types behave as before (they have by default isNullable=false, i.e. NOT NULL constraint), but we can now create struct types with isNullable=true via {{createTypeWithNullability(recordType, true)}} method (which so far did not change the nullability of the struct type itself). This means that, in order to have a nullable struct type, we'd need to go through createTypeWithNullability, either explicitly or implicitly (e.g. during a join result type computation).

However, the default behavior does not change, which means that the code proposed by [~zabetak] in the comment above (or any similar test added to type.iq) will still fail. In order to make it work, I think we would need to change the default behavior, and start creating nullable struct types from the beginning (perhaps the new default should be isNullable=true?), for example in JavaTypeFactoryImpl.java:
{code}
public RelDataType createType(Type type) {
  ...
  // return createStructType(clazz); // old code
  return createTypeWithNullability(createStructType(clazz), true); // new code?
  ...
}
{code}
But this change would introduce some side effects (many unit tests fail, maybe backwards compatibility might be an issue?). I am willing to carry on the task and fix the tests that will need to be modified.

> Allow to set nullability for columns of structured types
> --------------------------------------------------------
>
>                 Key: CALCITE-2464
>                 URL: https://issues.apache.org/jira/browse/CALCITE-2464
>             Project: Calcite
>          Issue Type: Bug
>          Components: core
>    Affects Versions: 1.17.0
>            Reporter: Stamatis Zampetakis
>            Assignee: Julian Hyde
>            Priority: Major
>
> Struct types are always not nullable. This can lead to bugs in many parts of Calcite (e.g., expression simplification, optimization, code generation) that are considering the nullability of a RelDataType. 
> The method [isNullable|https://github.com/apache/calcite/blob/3c6b5ec759caadabb67f09d7a4963cc7d9386d0c/core/src/main/java/org/apache/calcite/rel/type/RelRecordType.java#L55] in the RelRecordType, which is used to represent a structured type, always returns false. The nullability of the RelRecordType should be a parameter in the constructor as it is the case for various other RelDataTypes. 
> Additionally, the data type cache should also take into account the nullability of the type in order to return a correct equivalent.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)