Ensure "nessie.commit.id table" property is set when updating the table #19524
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ajantha-bhat
commented
Oct 30, 2023
...roduct-tests/src/main/java/io/trino/tests/product/iceberg/TestIcebergSparkCompatibility.java
Show resolved
Hide resolved
ajantha-bhat
commented
Oct 30, 2023
| onSpark().executeQuery("CREATE TABLE " + sparkTableName + " (a INT, b INT, c INT) USING ICEBERG"); | ||
| onSpark().executeQuery("INSERT INTO " + sparkTableName + " VALUES (1, 2, 3)"); | ||
|
|
||
| onTrino().executeQuery("INSERT INTO " + trinoTableName + " VALUES (4, 5, 6)"); |
There was a problem hiding this comment.
This used to fail for Nessie and there was no test case to cover this case.
|
cc: @findepi, @ebyhr, @findinpath |
|
@findinpath and @findepi: Can I please get a review on this? This is a small PR. |
nastra
approved these changes
Nov 13, 2023
findinpath
reviewed
Nov 14, 2023
...no-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractIcebergTableOperations.java
Show resolved
Hide resolved
| String sparkTableName = sparkTableName(baseTableName); | ||
| String trinoTableName = trinoTableName(baseTableName); | ||
|
|
||
| onSpark().executeQuery("CREATE TABLE " + sparkTableName + " (a INT, b INT, c INT) USING ICEBERG"); |
There was a problem hiding this comment.
i think we can stick to one column - there's no need to use multiple columns to test this change.
| String trinoTableName = trinoTableName(baseTableName); | ||
|
|
||
| onSpark().executeQuery("CREATE TABLE " + sparkTableName + " (a INT, b INT, c INT) USING ICEBERG"); | ||
| onSpark().executeQuery("INSERT INTO " + sparkTableName + " VALUES (1, 2, 3)"); |
There was a problem hiding this comment.
Do you want to ensure that the nessie.commit.id gets updated ?
public static String getNessieCommitId(String tableName)
{
String propertiesTableName = "\"" + baseTableName + "$properties\"";
String trinoPropertiesTableName = trinoTableName(propertiesTableName);
return (String) onTrino()
.executeQuery("SELECT value FROM " + trinoPropertiesTableName + " WHERE key = 'nessie.commit.id'")
.getOnlyValue();
}
There was a problem hiding this comment.
This test is common for other catalogs (groups) too. So, I didn't want to have a catalog (group) specific checks.
ajantha-bhat
changed the title
findinpath
reviewed
Nov 20, 2023
Comment on lines
997
to
999
| onSpark().executeQuery("INSERT INTO " + sparkTableName + " VALUES (1)"); | ||
|
|
||
| onTrino().executeQuery("INSERT INTO " + trinoTableName + " VALUES (2)"); |
There was a problem hiding this comment.
Suggested change
| onSpark().executeQuery("INSERT INTO " + sparkTableName + " VALUES (1)"); | |
| onTrino().executeQuery("INSERT INTO " + trinoTableName + " VALUES (2)"); | |
| onSpark().executeQuery("INSERT INTO " + sparkTableName + " VALUES 1"); | |
| onTrino().executeQuery("INSERT INTO " + trinoTableName + " VALUES 2"); |
Spark sets the table property NESSIE_COMMIT_ID_PROPERTY in NessieTableOperations#loadTableMetadata. Then NessieIcebergClient.commitTable uses this property. In Trino, this property is never set but used in NessieIcebergClient.commitTable as it is a common code. Hence, the commit id is old and doesn't allow new commits. Use the common code (available From Iceberg 1.4.0) NessieUtil.updateTableMetadataWithNessieSpecificProperties in Trino, which handles setting the property like "nessie.commit.id".
ajantha-bhat
changed the title
findinpath
approved these changes
Nov 21, 2023
|
The failed test |
electrum
approved these changes
Dec 15, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Spark sets the table property
NESSIE_COMMIT_ID_PROPERTYinNessieTableOperations#loadTableMetadata. ThenNessieIcebergClient.commitTableuses this property.In Trino, this property is never set but used in
NessieIcebergClient.commitTableas it is a common code. Hence, the commit id is old and doesn't allow new commits.Use the common code (available From Iceberg 1.4.0)
NessieUtil.updateTableMetadataWithNessieSpecificPropertiesin Trino, which handles setting the property like"nessie.commit.id".Additional context and related issues
Fixes #17813
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: