Skip to content
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 #3513

Conversation

jerryshao
Copy link
Contributor

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

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.

… 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.
@FANNG1
Copy link
Contributor

FANNG1 commented May 22, 2024

@caican00 could you fix the error and propose a new PR based on branch-0.5?

@caican00
Copy link
Collaborator

@caican00 could you fix the error and propose a new PR based on branch-0.5?

ok.

@jerryshao
Copy link
Contributor Author

I will close this. @caican00 please help to cherry-pick this commit manually with a new PR.

@jerryshao jerryshao closed this May 23, 2024
@jerryshao jerryshao deleted the cherry-pick-branch-0.5-4d6350aed2334e176b1c4a92f1974d5210462720 branch May 23, 2024 08:35
@caican00
Copy link
Collaborator

I will close this. @caican00 please help to cherry-pick this commit manually with a new PR.

done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants