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

[Bug report] bucket and truncate are missing width params in Gravitino SortOrder #2921

Closed
caican00 opened this issue Apr 13, 2024 · 6 comments · Fixed by #2928
Closed

[Bug report] bucket and truncate are missing width params in Gravitino SortOrder #2921

caican00 opened this issue Apr 13, 2024 · 6 comments · Fixed by #2928
Assignees
Labels
0.5.1 Release v0.5.1 0.6.0 Release v0.6.0 bug Something isn't working

Comments

@caican00
Copy link
Collaborator

Version

main branch

Describe what's wrong

bucket and truncate are missing width params in Gravitino SortOrder, as shown in the following picture:
image

Error message and/or stacktrace

None

How to reproduce

None

Additional context

No response

@caican00 caican00 added the bug Something isn't working label Apr 13, 2024
caican00 added a commit to caican00/gravitino that referenced this issue Apr 13, 2024
…et and truncate functions in Gravitino SortOrder
caican00 added a commit to caican00/gravitino that referenced this issue Apr 13, 2024
…et and truncate functions in Gravitino SortOrder
@FANNG1
Copy link
Contributor

FANNG1 commented Apr 16, 2024

could you provide more context about why should we add bucket and truncate to SortOrder? I'm also confused about the picture about the content. cc @yuqi1129

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 18, 2024

@caican00 could you provide more information here?

@caican00
Copy link
Collaborator Author

caican00 commented Apr 18, 2024

@caican00 could you provide more information here?

could you provide more context about why should we add bucket and truncate to SortOrder? I'm also confused about the picture about the content. cc @yuqi1129

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)

@caican00
Copy link
Collaborator Author

caican00 commented Apr 18, 2024

@FANNG1 @yuqi1129 I added some detailed information about the issue. If I missed anything, please help point it out. Thank you

@caican00
Copy link
Collaborator Author

@caican00 could you provide more information here?

could you provide more context about why should we add bucket and truncate to SortOrder? I'm also confused about the picture about the content. cc @yuqi1129

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)

@FANNG1 may i ask there is anything else I need to provide?

@FANNG1
Copy link
Contributor

FANNG1 commented Apr 19, 2024

ok, I got the context, will review it later.

@jerryshao jerryshao added this to the Gravitino June Release milestone Apr 24, 2024
FANNG1 pushed a commit that referenced this issue May 22, 2024
… 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.
github-actions bot pushed a commit that referenced this issue May 22, 2024
… 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.
caican00 added a commit to caican00/gravitino that referenced this issue May 23, 2024
…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.
jerryshao pushed a commit that referenced this issue May 23, 2024
… truncate functions in Gravitino SortOrder (#3530)

### 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.
@jerryshao jerryshao added 0.5.1 Release v0.5.1 0.6.0 Release v0.6.0 labels May 23, 2024
diqiu50 pushed a commit to diqiu50/gravitino that referenced this issue Jun 13, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.5.1 Release v0.5.1 0.6.0 Release v0.6.0 bug Something isn't working
Projects
None yet
3 participants