gravitino
gravitino copied to clipboard
apache
[#2921] fix(catalog-lakehouse-iceberg): Add width param to bucket and truncate functions in Gravitino SortOrder
What changes were proposed in this pull request?
Add width param to bucket and truncate functions in Gravitino SortOrder.
for example:
change bucket([id]) to bucket(10, id)
change truncate([name]) to truncate(2, name)
Why are the changes needed?
SortOrder in Iceberg supports FunctionExpression, such as year, month, bucket, truncate, etc.
truncate and bucket functions both have two parameters, such as bucket(10, col1), truncate(2, col2).
However, in gravitino, when converting an iceberg sortorder with bucket or truncate to gravitino sortOrder, there is only one parameter in bucket and truncate functions.
This picture shows the details of the parameters of the bucket and truncate functions in gravitino sortorder, we can test it in com.datastrato.gravitino.catalog.lakehouse.iceberg.converter.TestFromIcebergSortOrder#testFromSortOrder
And if we want to convert the gravitino sortOrder with bucket or truncate to iceberg sortOrder, we will get the following error as the first param is missing.
java.lang.IllegalArgumentException: Bucket sort should have 2 arguments
at com.google.common.base.Preconditions.checkArgument(Preconditions.java:143)
at com.datastrato.gravitino.catalog.lakehouse.iceberg.converter.ToIcebergSortOrder.toSortOrder(ToIcebergSortOrder.java:56)
at com.datastrato.gravitino.catalog.lakehouse.iceberg.converter.TestFromIcebergSortOrder.testFromSortOrder(TestFromIcebergSortOrder.java:86)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:498)
at org.junit.platform.commons.util.ReflectionUtils.invokeMethod(ReflectionUtils.java:725)
at org.junit.jupiter.engine.execution.MethodInvocation.proceed(MethodInvocation.java:60)
Fix: https://github.com/datastrato/gravitino/issues/2921
Does this PR introduce any user-facing change?
No
How was this patch tested?
New UTs and ITs.
@caican00 , sorry for the delay, this PR is related to API which we should check it carefully.
@caican00 , sorry for the delay, this PR is related to API which we should check it carefully.
It seems that i only added a new api declared as SortOrder functionSortOrder( String name, int width, int id, SortDirection direction, NullOrder nullOrder). cc @FANNG1
gently ping @Clearvive, could you help review this PR when you are free? Thank you
gently ping @Clearvive, could you help review this PR when you are free? Thank you
@caican00 , Clearvive if not focused on Gravitino now
gently ping @Clearvive, could you help review this PR when you are free? Thank you
@caican00 , Clearvive if not focused on Gravitino now
got it, thank you @FANNG1 is there anyone else who can help review this PR?
gently ping @Clearvive, could you help review this PR when you are free? Thank you
@caican00 , Clearvive if not focused on Gravitino now
got it, thank you @FANNG1 is there anyone else who can help review this PR?
@yuqi1129 and I could review this, but before viewing we'd better keep consistent about the necessary to do this in #2921
@mchades @FANNG1 all comments have been addressed, and could you please help review again? Thanks!
@FANNG1 all comments have been addressed, could you help review again? Thanks!