-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Support setting a field type with SET DATA TYPE
statement in engine and Iceberg
#18395
Conversation
783327d
to
9e4b521
Compare
4982e49
to
31e4be9
Compare
String caseSensitiveParentName = icebergTable.schema().findColumnName(parent.fieldId()); | ||
NestedField field = parent.type().asStructType().caseInsensitiveField(getLast(fieldPath)); | ||
if (!field.type().isPrimitiveType()) { | ||
throw new TrinoException(NOT_SUPPORTED, "Iceberg doesn't support changing field type from non-primitive types"); |
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.
Do we want to disallow type changes between primitive and nested at engine level?
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.
Even if we do this, connector will have to check that as well. Engine cannot do such checks atomically.
throw new TrinoException(NOT_SUPPORTED, "Unsupported type: " + type); | ||
} | ||
List<RowType.Field> candidates = rowType.getFields().stream() | ||
// case-insensitive match |
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.
Can this be case sensitive? (eg not to get us one step further from #17)
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 is the same logic as row fields resolution in expressions as you may know.
@kasiafi requested a case-insensitive match in adding fields PR #16321 (comment).
Currently, adding & renaming fields are case-insensitive and dropping field (this was the 1st PR about managing nested fields) is case-sensitive. We should change dropping fields to case-insensitive in the near future.
core/trino-spi/src/main/java/io/trino/spi/connector/ConnectorMetadata.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
String caseSensitiveParentName = icebergTable.schema().findColumnName(parent.fieldId()); | ||
NestedField field = parent.type().asStructType().caseInsensitiveField(getLast(fieldPath)); | ||
if (!field.type().isPrimitiveType()) { | ||
throw new TrinoException(NOT_SUPPORTED, "Iceberg doesn't support changing field type from non-primitive types"); |
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.
Even if we do this, connector will have to check that as well. Engine cannot do such checks atomically.
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergMetadata.java
Show resolved
Hide resolved
31e4be9
to
4c6319e
Compare
Description
This is the final PR about managing nested fields in the engine.
Fixes #16959
Release notes
(x) Release notes are required, with the following suggested text: