[#4757]feat(trino-connector): Support more partition and sort order features of the Iceberg catalog#4925
Conversation
diqiu50
changed the title
diqiu50
requested review from
mchades and
yuqi1129
and removed request for
yuqi1129
| } | ||
|
|
||
| tasks.build { | ||
| dependsOn("setupDependencies") |
There was a problem hiding this comment.
why build dependens on setupDependencies ?
There was a problem hiding this comment.
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.
how to check the partition or sort order takes effect?
There was a problem hiding this comment.
In the Trino , we can sue the command SHOW CREATE TABLE to check it.
| 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.
suggest to use a more meaning ful name, like toGravitinoPartition
There was a problem hiding this comment.
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.
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.
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.
use a more meaningful name like toTrinoSortOrder?
| } | ||
| } | ||
|
|
||
| private static void parseTransform(List<Transform> transforms, String value) { |
There was a problem hiding this comment.
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.
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? |
|
|
||
| private static void parseTransform(List<Transform> transforms, String value) { | ||
| boolean match = | ||
| false |
…rder features of the Iceberg catalog (apache#4925) ### 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: apache#4757 ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? New UTs and ITs
4 participants
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