zabetak commented on a change in pull request #898: [CALCITE-2464] Allow to set
nullability for columns of structured types (Ruben Quesada Lopez)
URL: https://github.com/apache/calcite/pull/898#discussion_r258515553
##########
File path: server/src/main/java/org/apache/calcite/sql/ddl/SqlCreateTable.java
##########
@@ -176,6 +176,9 @@ public void execute(CalcitePrepare.Context context) {
type = typeEntry.getType().apply(typeFactory);
}
}
+ if (type != null && !type.isNullable() && d.strategy ==
ColumnStrategy.NULLABLE) {
Review comment:
@rubenada I have a few concerns with the following lines.
First of all, is it possible to arrive here with a type==null? I guess not
so I think the first condition can go away and possibly add an assertion.
Second the nullability of simple types is already set in
d.dataType.deriveType (a few lines above). I can see that the code there does
not handle at all struct types since this is done by TypeEntry. I am wondering
why it is done like that but maybe the best person to answer this would be
@suez1224 .
Third, I see that for simple types the nullability information is set by
considering first SqlDataTypeSpec.nullable field and then the boolean parameter
passed in the method deriveType. ColumnStrategy is not used for this purpose so
I am wondering if it is correct to set the nullability based on this.
Finally, I am thinking that if deriveType cannot handle struct types then
maybe the nullability info (for structs) should be set in
typeEntry.getType().apply(typeFactory).
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
