[Iceberg] deprecate old table property names #24581

ZacBlanco · 2025-02-17T19:07:25Z

Description

Deprecates presto-specific table property names in favor of using the Iceberg library property names

Motivation and Context

Having a separate set of property names for iceberg is confusing for users

Impact

Iceberg library property names can be used, old names can also still be used
queries which alter table properties or create tables using old property names are given a deprecation warning
after a few releases, we will remove the old property names

Test Plan

new tests verify old and new properties can all be used
new tests verify warnings are added when any deprecated properties are used

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

== RELEASE NOTES ==

Iceberg Connector Changes
* Deprecate some table property names in favor of property names from the Iceberg library. See :doc:`connector/iceberg.rst`.

steveburnett · 2025-02-17T19:11:48Z

Thanks for the release note! Nit rephrasing suggestion:

== RELEASE NOTES ==

Iceberg Connector Changes
* Replace some deprecated table property names with property names from the Iceberg library. See :doc:`connector/iceberg.rst`.

ZacBlanco · 2025-02-17T19:16:12Z

Replace some deprecated table property names with property names from the Iceberg library. See :doc:connector/iceberg.rst

This is actually slightly incorrect, because the properties were not deprecated before this change. I modified your suggestion to make it clear that this change is the start of the deprecation of the property names

This commit introduces the deprecation of a few tables property names in favor of the iceberg library's naming scheme. It is to help reduce confusion for users as to which table properties in Iceberg documentation map to presto. Users will get a warning on any queries which set table properties such as CREATE TABLE and ALTER TABLE .. SET PROPERTIES statements. Refer to IcebergTableProperties.java or iceberg.rst for the list of deprecated names and their corresponding mapping.

steveburnett

LGTM! (docs)

Pull branch, new doc build, looks good. Thanks for the docs!

yingsu00 · 2025-02-24T22:14:39Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableProperties.java

    public static final String FILE_FORMAT_PROPERTY = "format";
+    private static final String DEFAULT_FORMAT_VERSION = "2";


Move to private static section below the public and protected ones

yingsu00 · 2025-02-24T22:19:21Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableProperties.java


    private final List<PropertyMetadata<?>> tableProperties;
    private final List<PropertyMetadata<?>> columnProperties;

+    @VisibleForTesting
+    protected static final BiMap<String, String> DEPRECATED_PROPERTIES = ImmutableBiMap.<String, String>builder()


I don't see where this is directly used outside of IcebergTableProperties.java. Why do you mark it VisibleForTesting and protected?

yingsu00 · 2025-02-24T22:23:12Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergTableProperties.java

+            .build();
+
+    @VisibleForTesting
+    protected static final Set<String> UPDATABLE_PROPERTIES = ImmutableSet.<String>builder()


ditto, why protected?

yingsu00 · 2025-02-24T22:39:38Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergWarningCode.java

+{
+    SORT_COLUMN_TRANSFORM_NOT_SUPPORTED_WARNING(1),
+    USE_OF_DEPRECATED_TABLE_PROPERTY(2);
+    private final WarningCode warningCode;


I know existing code also has this, but private final should come after "public static final" or even "private static final"

yingsu00 · 2025-02-24T22:40:03Z

presto-iceberg/src/main/java/com/facebook/presto/iceberg/IcebergWarningCode.java

+    SORT_COLUMN_TRANSFORM_NOT_SUPPORTED_WARNING(1),
+    USE_OF_DEPRECATED_TABLE_PROPERTY(2);
+    private final WarningCode warningCode;
+    public static final int WARNING_CODE_MASK = 0x0200_0000;


this can be "private static final"

Also we will need to sort out the WARNING_CODE_MASK's. I see ParquetWarningCode and HiveWarningCode are using the same value and they may clash. The intention for the warning code is to be unique.

I am aware they should be unique. I made this value different than the existing warnings code masks which I was able to find in the codebase. We should fix the masks for Hive and Parquet in a separate PR though

yingsu00 · 2025-02-24T22:43:13Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/rest/TestIcebergSmokeRest.java

+    @Override
+    public void testDeprecatedTablePropertiesCreateTable()
+    {
+        // fails due to bug in earlier iceberg versions. Remove when iceberg library is updated


What was the error? Could you please add a little more information about the error here?

It's a bug in the RestCatalog implementation which makes it impossible to create a table with format-version set to 1.

yingsu00 · 2025-02-24T22:46:59Z

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedTestBase.java

@@ -203,14 +203,14 @@ protected QueryRunner createQueryRunner()
    public void testDeleteOnV1Table()
    {
        // Test delete all rows
-        long totalCount = (long) getQueryRunner().execute("CREATE TABLE test_delete with (format_version = '1') as select * from lineitem")
+        long totalCount = (long) getQueryRunner().execute("CREATE TABLE test_delete with (\"format-version\" = '1') as select * from lineitem")


Can we add some tests with the old names in which they should also succeed but with a warning?

That's what these two tests are for:

presto/presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

Lines 1948 to 1990 in d1ac6aa

@Test

public void testDeprecatedTablePropertiesCreateTable()

{

Map<String, String> deprecatedProperties = ImmutableMap.<String, String>builder()

.put(FILE_FORMAT_PROPERTY, "'ORC'")

.put(FORMAT_VERSION, "'1'")

.put(COMMIT_RETRIES, "1234")

.put(DELETE_MODE, "'copy-on-write'")

.put(METADATA_PREVIOUS_VERSIONS_MAX, "567")

.put(METADATA_DELETE_AFTER_COMMIT, "true")

.put(METRICS_MAX_INFERRED_COLUMN, "123")

.build();

deprecatedProperties.forEach((oldProperty, value) -> {

Session session = Session.builder(getSession()).build();

String tableName = "test_deprecated_table_properties_" + oldProperty;

DistributedQueryRunner queryRunner = (DistributedQueryRunner) getQueryRunner();

MaterializedResult result = queryRunner.execute(session, "CREATE TABLE " + tableName + " (c1 integer) WITH (" + oldProperty + " = " + value + ")");

assertEquals(result.getWarnings().size(), 1);

assertTrue(result.getWarnings().stream()

.anyMatch(code -> code.getWarningCode().equals(USE_OF_DEPRECATED_TABLE_PROPERTY.toWarningCode())));

assertUpdate(session, "DROP TABLE " + tableName);

});

}

@Test

public void testDeprecatedTablePropertiesAlterTable()

{

Map<String, String> deprecatedProperties = ImmutableMap.<String, String>builder()

.put(COMMIT_RETRIES, "1234")

.build();

deprecatedProperties.forEach((oldProperty, value) -> {

Session session = Session.builder(getSession()).build();

String tableName = "test_deprecated_table_properties_" + oldProperty;

DistributedQueryRunner queryRunner = (DistributedQueryRunner) getQueryRunner();

assertQuerySucceeds(session, "CREATE TABLE " + tableName + " (c1 integer) WITH (" + oldProperty + " = " + value + ")");

MaterializedResult result = queryRunner.execute(session, "ALTER TABLE " + tableName + " SET PROPERTIES (" + oldProperty + " = " + value + ")");

assertEquals(result.getWarnings().size(), 1);

assertTrue(result.getWarnings().stream()

.anyMatch(code -> code.getWarningCode().equals(USE_OF_DEPRECATED_TABLE_PROPERTY.toWarningCode())));

assertUpdate(session, "DROP TABLE " + tableName);

});

}

}

prestodb-ci added the from:IBM PR from IBM label Feb 17, 2025

ZacBlanco mentioned this pull request Feb 17, 2025

[Iceberg] Add table and session property for split size #24417

Merged

6 tasks

ZacBlanco force-pushed the upstream-iceberg-deprecate-table-properties branch 2 times, most recently from c3e534e to e3cdebc Compare February 20, 2025 20:12

ZacBlanco force-pushed the upstream-iceberg-deprecate-table-properties branch from e3cdebc to d1ac6aa Compare February 20, 2025 23:18

ZacBlanco marked this pull request as ready for review February 21, 2025 07:03

ZacBlanco requested review from steveburnett, elharo, hantangwangd and a team as code owners February 21, 2025 07:03

ZacBlanco requested a review from presto-oss February 21, 2025 07:03

steveburnett approved these changes Feb 21, 2025

View reviewed changes

yingsu00 reviewed Feb 24, 2025

View reviewed changes

yingsu00 added the RELEASE-BLOCKER label Mar 12, 2025

prestodb-ci requested review from a team, sh-shamsan and pdabre12 and removed request for a team March 12, 2025 02:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Iceberg] deprecate old table property names #24581

[Iceberg] deprecate old table property names #24581

ZacBlanco commented Feb 17, 2025 •

edited

Loading

steveburnett commented Feb 17, 2025

ZacBlanco commented Feb 17, 2025 •

edited

Loading

steveburnett left a comment

yingsu00 Feb 24, 2025

yingsu00 Feb 24, 2025

yingsu00 Feb 24, 2025

yingsu00 Feb 24, 2025

yingsu00 Feb 24, 2025

yingsu00 Feb 24, 2025

ZacBlanco Feb 24, 2025

yingsu00 Feb 24, 2025

ZacBlanco Feb 24, 2025

yingsu00 Feb 24, 2025

ZacBlanco Feb 24, 2025

		public static final String FILE_FORMAT_PROPERTY = "format";
		private static final String DEFAULT_FORMAT_VERSION = "2";

	@Test
	public void testDeprecatedTablePropertiesCreateTable()
	{
	Map<String, String> deprecatedProperties = ImmutableMap.<String, String>builder()
	.put(FILE_FORMAT_PROPERTY, "'ORC'")
	.put(FORMAT_VERSION, "'1'")
	.put(COMMIT_RETRIES, "1234")
	.put(DELETE_MODE, "'copy-on-write'")
	.put(METADATA_PREVIOUS_VERSIONS_MAX, "567")
	.put(METADATA_DELETE_AFTER_COMMIT, "true")
	.put(METRICS_MAX_INFERRED_COLUMN, "123")
	.build();
	deprecatedProperties.forEach((oldProperty, value) -> {
	Session session = Session.builder(getSession()).build();
	String tableName = "test_deprecated_table_properties_" + oldProperty;
	DistributedQueryRunner queryRunner = (DistributedQueryRunner) getQueryRunner();
	MaterializedResult result = queryRunner.execute(session, "CREATE TABLE " + tableName + " (c1 integer) WITH (" + oldProperty + " = " + value + ")");
	assertEquals(result.getWarnings().size(), 1);
	assertTrue(result.getWarnings().stream()
	.anyMatch(code -> code.getWarningCode().equals(USE_OF_DEPRECATED_TABLE_PROPERTY.toWarningCode())));
	assertUpdate(session, "DROP TABLE " + tableName);
	});
	}

	@Test
	public void testDeprecatedTablePropertiesAlterTable()
	{
	Map<String, String> deprecatedProperties = ImmutableMap.<String, String>builder()
	.put(COMMIT_RETRIES, "1234")
	.build();
	deprecatedProperties.forEach((oldProperty, value) -> {
	Session session = Session.builder(getSession()).build();
	String tableName = "test_deprecated_table_properties_" + oldProperty;
	DistributedQueryRunner queryRunner = (DistributedQueryRunner) getQueryRunner();
	assertQuerySucceeds(session, "CREATE TABLE " + tableName + " (c1 integer) WITH (" + oldProperty + " = " + value + ")");
	MaterializedResult result = queryRunner.execute(session, "ALTER TABLE " + tableName + " SET PROPERTIES (" + oldProperty + " = " + value + ")");
	assertEquals(result.getWarnings().size(), 1);
	assertTrue(result.getWarnings().stream()
	.anyMatch(code -> code.getWarningCode().equals(USE_OF_DEPRECATED_TABLE_PROPERTY.toWarningCode())));
	assertUpdate(session, "DROP TABLE " + tableName);
	});
	}
	}

[Iceberg] deprecate old table property names #24581

Are you sure you want to change the base?

[Iceberg] deprecate old table property names #24581

Conversation

ZacBlanco commented Feb 17, 2025 • edited Loading

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

steveburnett commented Feb 17, 2025

ZacBlanco commented Feb 17, 2025 • edited Loading

steveburnett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ZacBlanco commented Feb 17, 2025 •

edited

Loading

ZacBlanco commented Feb 17, 2025 •

edited

Loading