-
Notifications
You must be signed in to change notification settings - Fork 319
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
[#2921] fix(catalog-lakehouse-iceberg): Add width param to bucket and truncate functions in Gravitino SortOrder #2928
Conversation
…et and truncate functions in Gravitino SortOrder
…et and truncate functions in Gravitino SortOrder
…to iceberg-properties
…le formats in using clause when create iceberg tables
…le formats in using clause when create iceberg tables
...in/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergSortOrder.java
Show resolved
Hide resolved
Hi @FANNG1 could you help review this PR when you are free? Thanks |
api/src/main/java/com/datastrato/gravitino/rel/expressions/FunctionExpression.java
Outdated
Show resolved
Hide resolved
…to iceberg-properties
… into iceberg-properties
Hi @FANNG1 do you have time to review this PR? thanks |
Hi @Clearvive could you help review this PR? Thank you |
@caican00 , sorry for the delay, this PR is related to API which we should check it carefully. |
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 |
Hi @yuqi1129 could you help review this pr if you are free? Thank you. |
@FANNG1 could you please help review this PR? Thank you! |
ok |
...in/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/ToIcebergSortOrder.java
Show resolved
Hide resolved
...ava/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestToIcebergSortOrder.java
Outdated
Show resolved
Hide resolved
...eberg/src/test/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/TestIcebergTable.java
Outdated
Show resolved
Hide resolved
...om/datastrato/gravitino/catalog/lakehouse/iceberg/integration/test/CatalogIcebergBaseIT.java
Outdated
Show resolved
Hide resolved
.../test/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestBaseConvert.java
Outdated
Show resolved
Hide resolved
.../test/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestBaseConvert.java
Outdated
Show resolved
Hide resolved
.../test/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestBaseConvert.java
Outdated
Show resolved
Hide resolved
...a/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestFromIcebergSortOrder.java
Outdated
Show resolved
Hide resolved
.../test/java/com/datastrato/gravitino/catalog/lakehouse/iceberg/converter/TestBaseConvert.java
Outdated
Show resolved
Hide resolved
...om/datastrato/gravitino/catalog/lakehouse/iceberg/integration/test/CatalogIcebergBaseIT.java
Outdated
Show resolved
Hide resolved
LGTM except the comment, @mchades do you have time to review again? |
ace6cae
to
28dfe4b
Compare
@FANNG1 all comments have been addressed, could you help review again? Thanks! |
… truncate functions in Gravitino SortOrder (#2928) ### 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` ![image](https://github.com/datastrato/gravitino/assets/94670132/06fd65f5-7d33-4197-8871-dda02fd70a26) 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: #2921 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New UTs and ITs.
…et and truncate functions in Gravitino SortOrder (apache#2928) 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)` 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` ![image](https://github.com/datastrato/gravitino/assets/94670132/06fd65f5-7d33-4197-8871-dda02fd70a26) 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: apache#2921 No New UTs and ITs.
…et and truncate functions in Gravitino SortOrder (apache#2928) ### 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` ![image](https://github.com/datastrato/gravitino/assets/94670132/06fd65f5-7d33-4197-8871-dda02fd70a26) 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: apache#2921 ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? New UTs and ITs.
What changes were proposed in this pull request?
Add width param to bucket and truncate functions in Gravitino SortOrder.
for example:
change
bucket([id])
tobucket(10, id)
change
truncate([name])
totruncate(2, name)
Why are the changes needed?
SortOrder in Iceberg supports
FunctionExpression
, such asyear, month, bucket, truncate, etc
.truncate
andbucket
functions both have two parameters, such asbucket(10, col1), truncate(2, col2)
.However, in gravitino, when converting an iceberg sortorder with
bucket
ortruncate
to gravitino sortOrder, there is only one parameter inbucket
andtruncate
functions.This picture shows the details of the parameters of the
bucket
andtruncate
functions in gravitino sortorder, we can test it incom.datastrato.gravitino.catalog.lakehouse.iceberg.converter.TestFromIcebergSortOrder#testFromSortOrder
And if we want to convert the gravitino sortOrder with
bucket
ortruncate
to iceberg sortOrder, we will get the following error as the first param is missing.Fix: #2921
Does this PR introduce any user-facing change?
No
How was this patch tested?
New UTs and ITs.