[CALCITE-6764] Field access from a nullable ROW should be nullable (part 2) #4139
Conversation
There was a problem hiding this comment.
Nothing major but I am confused on a couple of points @mihaibudiu, can you take a look?
core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
Outdated
Show resolved
Hide resolved
|
@asolimando I have pushed a commit which hopefully addresses your comments. |
|
I will write a few more tests, so I will mark this PR as draft until I am sure it passes all of them. Hopefully this won't take too long. |
Thanks @mihaibudiu, LGTM, just proposed two improved test names, the rest is good, feel free to ping me for an extra review round once the extra tests are ready, if needed |
core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/apache/calcite/test/TableInRootSchemaTest.java
Outdated
Show resolved
Hide resolved
|
The complete fix required some additions to the type factory interfaces and making a method public. |
|
@asolimando I made some changes that probably will require a new review |
There was a problem hiding this comment.
Looks mostly good to me @mihaibudiu, just confused about the fact that we are not considering anything other than Records in RelDataTypeFactoryImpl, if you could clarify that we are good for me.
| @@ -179,7 +179,9 @@ RelDataType createMultisetType( | |||
| * Creates a type that is the same as another type but with possibly | |||
| * different nullability. The output type may be identical to the input | |||
| * type. For type systems without a concept of nullability, the return value | |||
| * is always the same as the input. | |||
| * is always the same as the input. This function never returns a nullable | |||
There was a problem hiding this comment.
| * is always the same as the input. This function never returns a nullable | |
| * is always the same as the input. This function never returns a nullable |
| * Creates a type that is the same as another type but with possibly | ||
| * different nullability. The output type may be identical to the input | ||
| * type. For type systems without a concept of nullability, the return value | ||
| * is always the same as the input. This differs from {@link |
There was a problem hiding this comment.
| * is always the same as the input. This differs from {@link | |
| * is always the same as the input. This differs from {@link |
| * type. For type systems without a concept of nullability, the return value | ||
| * is always the same as the input. This differs from {@link | ||
| * #createTypeWithNullability(RelDataType, boolean)} in the handling of struct | ||
| * types. This function returns a nullable struct. |
There was a problem hiding this comment.
| * types. This function returns a nullable struct. | |
| * types. This function returns a nullable struct. |
| * #createTypeWithNullability(RelDataType, boolean)} in the handling of struct | ||
| * types. This function returns a nullable struct. | ||
| * | ||
| * @param type input type |
There was a problem hiding this comment.
Nit: I see that some javadoc here has this kind of formatting with extra spaces to align columns "horizontally", AFAICT this is not what we generally use in the codebase, so I'd rather not enforce this here
| } | ||
| }, | ||
| type.getFieldNames(), nullable); | ||
| } else { |
There was a problem hiding this comment.
Aren't there other non simple types like maps (MultisetSqlType)/arrays (ArraySqlType)? Is it enough in this form?
EDIT: I have just seen you have done that for the SqlTypeFactoryImpl, why here it is not needed?
There was a problem hiding this comment.
As far as I can tell the other types are handled correctly by the factory.
Only Record types are anomalous.
This class (RelDataTypeFactoryImpl) is an abstract base class out of which SqlTypeFactoryImpl is derived. In the end, the SqlTypeFactoryImpl method gets called.
There was a problem hiding this comment.
Thanks for the clarification Mihai, I have no more questions, hence I will approve the PR, the rest is minor and I will leave it to your judgement
There was a problem hiding this comment.
thank you. I will address the comments and squash the commits at the same time.
I am not sure whether adding functions to an interface is a breaking change that has to be documented in history.md.
There was a problem hiding this comment.
I will document it anyway.
There was a problem hiding this comment.
I guess so, because implementers will have to add support for it
There was a problem hiding this comment.
I will add this to history.md in the section on breaking changes:
*RelDataTypeFactory interface*. The fix for [<a
href="https://issues.apache.org/jira/browse/CALCITE-6764">CALCITE-6764</a>]
introduces a new method
`RelDataTypeFactory#enforceTypeWithNullability` in the existing
`RelDataTypeFactory` interface. The behavior of the new function is
similar to the existing API `createTypeWithNullability`; however, the
existing implementations of the `createTypeWithNullability` API cannot
create nullable record (`ROW`) types. Nullable record types are
legitimate in several SQL dialects.
…art 2) Signed-off-by: Mihai Budiu <[email protected]>
mihaibudiu
added
the
LGTM-will-merge-soon
|
We have already merged a PR fixing this issue, but I have reopened it, and this is a second PR addressing a second bug related to the same problem.