-
Notifications
You must be signed in to change notification settings - Fork 378
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
[#2962] feat(spark-connector): Add reserved properties to Table Properties when load an Iceberg table #2964
Conversation
…le formats in using clause when create iceberg tables
…le formats in using clause when create iceberg tables
… into iceberg-load-properties
...test/java/com/datastrato/gravitino/integration/test/spark/iceberg/SparkIcebergCatalogIT.java
Show resolved
Hide resolved
...rk-connector/src/main/java/com/datastrato/gravitino/spark/connector/PropertiesConverter.java
Show resolved
Hide resolved
Hi @FANNG1 could you help review this PR when you are free? Thank you very much. |
Map<String, String> properties; | ||
properties = | ||
propertiesConverter.toSparkTableProperties( | ||
gravitinoTable.properties(), getSparkTable().properties()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems hacky to combine properties from Gravitino and realCatalog. I think the right direction is try to provide the properties from Gravitino.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know the reason why couldn't get the reserved properties from Gravitino?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems hacky to combine properties from Gravitino and realCatalog. I think the right direction is try to provide the properties from Gravitino.
My original implementation was this, but I was concerned that if all reserved properties were placed into Gravitino
IcebergTable
's properties, there would be differences between different computing engines, such as Spark
and Flink
.
Because they all will get the same reserved properties from the Gravitino
IcebergTable
.
For this reason, i changed the solution to retrieve reserved properties from the realTable at the spark-connector. cc @FANNG1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems hacky to combine properties from Gravitino and realCatalog. I think the right direction is try to provide the properties from Gravitino.
My original implementation was this, but I was concerned that if all reserved properties were placed into
Gravitino
IcebergTable
's properties, there would be differences between different computing engines, such asSpark
andFlink
. Because they all will get the same reserved properties from theGravitino
IcebergTable
. For this reason, i changed the solution to retrieve reserved properties from the realTable at the spark-connector. cc @FANNG1
@FANNG1 Should i add the reserved properties into the Gravitino IceebrgTable's properties directly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know the reason why couldn't get the reserved properties from Gravitino?
@caican00 do you know the reason?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@FANNG1 putting location into properties in Gravitino side will cause unexpected problems, such as trino.
https://github.com/datastrato/gravitino/actions/runs/8718294890/job/23915239099?pr=2709
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caican00
You can directly change the file lakehouse-iceberg/00000_create_table.txt
and use the correct output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@caican00 You can directly change the file
lakehouse-iceberg/00000_create_table.txt
and use the correct output.
got it.
close this pr as we have create a new one #3511 |
What changes were proposed in this pull request?
Add reserved properties to Table properties when load an Iceberg table, such as:
And we got the reserved properties from the proxy
SparkTable
ofIceberg
.Why are the changes needed?
for example, when execute
desc extended IcebergTableName
,it will get some information from the result properties of this Iceberg table, so it should contain the above properties.Fix: #2962
Does this PR introduce any user-facing change?
No.
How was this patch tested?
New UTs and ITs.