-
Notifications
You must be signed in to change notification settings - Fork 304
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
[#4757]feat(trino-connector): Support more partition and sort order features of the Iceberg catalog #4925
base: main
Are you sure you want to change the base?
Conversation
} | ||
|
||
tasks.build { | ||
dependsOn("setupDependencies") |
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.
why build dependens on setupDependencies
?
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.
Running the main class TrinoQueryTestTool
requires dependencies on other modules's jars.
By default, executing the main class depends on the build task in IDEA.
shipmode varchar, | ||
comment varchar | ||
) | ||
WITH ( |
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.
how to check the partition or sort order takes effect?
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.
In the Trino , we can sue the command SHOW CREATE TABLE
to check it.
@@ -126,20 +120,12 @@ public GravitinoTable createTable(ConnectorTableMetadata tableMetadata) { | |||
new GravitinoTable(schemaName, tableName, columns, comment, properties); | |||
|
|||
if (!partitionColumns.isEmpty()) { | |||
Transform[] partitioning = | |||
partitionColumns.stream().map(Transforms::identity).toArray(Transform[]::new); | |||
Transform[] partitioning = ExpressionUtil.partitionFiledToExpression(partitionColumns); |
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.
suggest to use a more meaning ful name, like toGravitinoPartition
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 you mean the function name or the parameter name.
if it's the function name . I think the name is meaningful . Actually, it transforms the partition field into an expression
return SortOrders.ascending(expression); | ||
}) | ||
.toArray(SortOrder[]::new); | ||
SortOrder[] sorting = ExpressionUtil.sortOrderFiledToExpression(sortColumns); |
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.
suggest to use a more meaning ful name, like toGravitinoSortOrder
.map(ts -> ((Transform.SingleFieldTransform) ts).fieldName()[0]) | ||
.collect(Collectors.toList()) | ||
: Collections.emptyList()); | ||
ExpressionUtil.expressionToPartitionFiled(gravitinoTable.getPartitioning())); |
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.
use a more meaningful name like toTrinoPartition
?
return ((NamedReference) expression).fieldName()[0]; | ||
}) | ||
.collect(Collectors.toList())); | ||
ExpressionUtil.expressionToSortOrderFiled(gravitinoTable.getSortOrders())); |
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.
use a more meaningful name like toTrinoSortOrder
?
} | ||
} | ||
|
||
private static void parseTransform(List<Transform> transforms, String value) { |
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.
it's hard to maintan, is there any any better way to implement this? how origin Iceberg connector handle this logic?
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 you have a better solution? the Iceberg connector handle this logic is the same as this.
@yuqi1129 , do you have time to review this PR? |
What changes were proposed in this pull request?
Support Iceberg partition expressions like: year(x), month(x), day(x), hour(x), bucket(x, n), truncate(x,n)
Support Iceberg sort order expressions like: field DESC, field ASC, field DESC NULLS FIRST, field ASC NULLS LAST
Why are the changes needed?
Fix: #4757
Does this PR introduce any user-facing change?
NO
How was this patch tested?
New UTs and ITs