-
Notifications
You must be signed in to change notification settings - Fork 13.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FLINK-20539][table-planner] Fix type mismatch when casting ROW #24994
base: master
Are you sure you want to change the base?
Conversation
createTestItem( | ||
"ROW<f0 INT NOT NULL, f1 BOOLEAN>", | ||
DataTypes.ROW( | ||
DataTypes.FIELD("f0", DataTypes.INT()), | ||
DataTypes.FIELD("f0", DataTypes.INT().notNull()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This hits the previous fix in
Line 433 in c226ea9
case rt: RelRecordType if rt.getStructKind == StructKind.PEEK_FIELDS_NO_EXPAND => |
// nullable and the cast does not allow setting the outer | ||
// nullability but derives it from the source operand | ||
DataTypes.of("ROW<r ROW<s STRING, b BOOLEAN, i INT>, s STRING>")), | ||
// For nested rows we keep the nullability property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
@@ -93,16 +93,18 @@ Stream<TestSetSpec> getTestSetSpecs() { | |||
FIELD( | |||
"r", | |||
ROW( | |||
FIELD("s", STRING()), | |||
FIELD( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have fixed here to keep types in sql and types in table api equivalent.
What is the purpose of the change
When using cast, Flink uses
ExtendedSqlRowTypeNameSpec
to derive the row type, and we should change theStructKind
fromFULLY_QUALIFIED
in Calcite toPEEK_FIELDS_NO_EXPAND
in Flink.Brief change log
Verifying this change
Some tests are added to verify this pr.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: noDocumentation