Add iceberg.default-new-tables-gc.enabled config property #27359

sopel39 · 2025-11-18T14:39:04Z

iceberg.default-new-tables-gc.enabled will set gc.enabled flag for new tables created by Trino.

Description

Additional context and related issues

Release notes

( ) 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.
(x) Release notes are required, with the following suggested text:

## Iceberg
* Honor and allow to set default value for `gc.enabled` flag ({issue}`issuenumber`)

iceberg.default-new-tables-gc.enabled will set gc.enabled flag for new tables created by Trino.

raunaqmorarka · 2025-11-18T17:38:57Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

+        return defaultNewTablesGcEnabled;
+    }
+
+    @Config("iceberg.default-new-tables-gc.enabled")


iceberg.default-new-tables.gc.enabled

raunaqmorarka · 2025-11-18T17:45:22Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java

+
+        // If user doesn't set gc.enabled, we need to set it to defaultNewTablesGcEnabled value
+        if (!extraProperties.containsKey(GC_ENABLED) && defaultNewTablesGcEnabled != GC_ENABLED_DEFAULT) {
+            propertiesBuilder.put(GC_ENABLED, Boolean.toString(defaultNewTablesGcEnabled));


One thing to note about relying on GC_ENABLED property of iceberg is that it will also disallow expire_snapshots.
Is that something desirable ?
I can imagine that some users just want protection on DROP, but with this they can't run the regular maintenance operation of expiring old snapshots

I guess they could enable GC for expire_snapshots. Users often have side spark anyway. Unless there is another property that would just limit DROP, then I think it's reasonable to expect data admins to know how to handle this situation.

There is a check in iceberg library org.apache.iceberg.RemoveSnapshots#RemoveSnapshots

ValidationException.check( PropertyUtil.propertyAsBoolean(base.properties(), GC_ENABLED, GC_ENABLED_DEFAULT), "Cannot expire snapshots: GC is disabled (deleting files may corrupt other tables)");

Using Spark or something else won't help with that.
I'm not sure how admin deals with this, adding and removing property before/after every expire_snapshots is cumbersome and removes the DROP protection for the duration of the command. Also, most ppl have automated maintenance services, and we wouldn't want ppl to have to go and modify that.

It looks like a breaking change to expire_snapshots?

But if other engine disable the gc flag, it's also a problem to Trino, user not able to expire snapshots due a unsupported or exposed property

ebyhr · 2025-11-19T03:52:23Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

                        .put("iceberg.file-format", format.name())
                        // Only allow some extra properties. Add "sorted_by" so that we can test that the property is disallowed by the connector explicitly.
-                        .put("iceberg.allowed-extra-properties", "extra.property.one,extra.property.two,extra.property.three,sorted_by")
+                        .put("iceberg.allowed-extra-properties", "extra.property.one,extra.property.two,extra.property.three,sorted_by,gc.enabled")


We use BaseIcebergConnectorSmokeTest or BaseTrinoCatalogTest when we verify catalog behavior. I recommend adding at least one positive test case to BaseIcebergConnectorSmokeTest.

ebyhr · 2025-11-19T03:52:25Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

    }

+    @Test
+    public void testDefaultGcEnabledTablePropertyOnCreateAndCtas() throws IOException


nit: Put throws IOException on a new line. Same for other tests.

ebyhr · 2025-11-19T03:52:33Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

+        return defaultNewTablesGcEnabled;
+    }
+
+    @Config("iceberg.default-new-tables-gc.enabled")


Could you document this property in iceberg.md?

findinpath · 2025-11-19T09:13:39Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

    private int planningThreads = Math.min(Runtime.getRuntime().availableProcessors(), 16);
    private int fileDeleteThreads = Runtime.getRuntime().availableProcessors() * 2;
    private List<String> allowedExtraProperties = ImmutableList.of();
+    private boolean defaultNewTablesGcEnabled = GC_ENABLED_DEFAULT;


The common approach is to restrain referencing in the config files values from dependent libraries because this may trigger a silent change in the default configuration of the connector.
Please consider using rather true instead

findinpath · 2025-11-19T09:15:43Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergConfig.java

+    }
+
+    @Config("iceberg.default-new-tables-gc.enabled")
+    @ConfigDescription("Default value for Iceberg property gc.enabled when creating new tables")


Let's add as well corresponding documentation in iceberg.md as well.
I noticed that the documentation for gc.enabled (introduced by apache/iceberg#1796 ) table property is unfortunately missing as well on apache/iceberg - As a reference point you could use apache/iceberg#9231

Property to disable garbage collection operations such as expiring snapshots or removing orphan files

It seems that in the meantime the table property has as well repercussions on whether to delete dropped table's files as well.

findinpath · 2025-11-19T09:17:05Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/glue/TrinoGlueCatalog.java

            LOG.warn(e, "Failed to delete table data referenced by metadata");
        }
-        deleteTableDirectory(fileSystemFactory.create(session), schemaTableName, table.location());
+        if (propertyAsBoolean(table.properties(), GC_ENABLED, GC_ENABLED_DEFAULT)) {


Pls replace GC_ENABLED_DEFAULT with true - we don't have control on the value of GC_ENABLED_DEFAULT when the iceberg library gets updated.

findinpath · 2025-11-19T09:17:37Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/hms/TrinoHiveCatalog.java

            log.warn(e, "Failed to delete table data referenced by metadata");
        }
-        deleteTableDirectory(fileSystemFactory.create(session), schemaTableName, metastoreTable.getStorage().getLocation());
+        if (propertyAsBoolean(metadata.properties(), GC_ENABLED, GC_ENABLED_DEFAULT)) {


Suggested change

if (propertyAsBoolean(metadata.properties(), GC_ENABLED, GC_ENABLED_DEFAULT)) {

if (propertyAsBoolean(metadata.properties(), GC_ENABLED, true)) {

findinpath · 2025-11-19T09:17:47Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/jdbc/TrinoJdbcCatalog.java

            LOG.warn(e, "Failed to delete table data referenced by metadata");
        }
-        deleteTableDirectory(fileSystemFactory.create(session), schemaTableName, table.location());
+        if (propertyAsBoolean(table.properties(), GC_ENABLED, GC_ENABLED_DEFAULT)) {


Suggested change

if (propertyAsBoolean(table.properties(), GC_ENABLED, GC_ENABLED_DEFAULT)) {

if (propertyAsBoolean(table.properties(), GC_ENABLED, true)) {

findinpath · 2025-11-19T09:18:03Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java

        PartitionSpec partitionSpec = parsePartitionFields(schema, getPartitioning(materializedViewProperties));
        SortOrder sortOrder = parseSortFields(schema, getSortOrder(materializedViewProperties));
-        Map<String, String> properties = createTableProperties(new ConnectorTableMetadata(storageTableName, columns, materializedViewProperties, Optional.empty()), _ -> false);
+        Map<String, String> properties = createTableProperties(new ConnectorTableMetadata(storageTableName, columns, materializedViewProperties, Optional.empty()), _ -> false, GC_ENABLED_DEFAULT);


Suggested change

Map<String, String> properties = createTableProperties(new ConnectorTableMetadata(storageTableName, columns, materializedViewProperties, Optional.empty()), _ -> false, GC_ENABLED_DEFAULT);

Map<String, String> properties = createTableProperties(new ConnectorTableMetadata(storageTableName, columns, materializedViewProperties, Optional.empty()), _ -> false, true);

findinpath · 2025-11-19T09:18:14Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/AbstractTrinoCatalog.java

        String tableLocation = getTableLocation(tableMetadata.getProperties())
                .orElseGet(() -> defaultTableLocation(session, tableMetadata.getTable()));
-        Transaction transaction = IcebergUtil.newCreateTableTransaction(this, tableMetadata, session, false, tableLocation, _ -> false);
+        Transaction transaction = IcebergUtil.newCreateTableTransaction(this, tableMetadata, session, false, tableLocation, _ -> false, GC_ENABLED_DEFAULT);


Suggested change

Transaction transaction = IcebergUtil.newCreateTableTransaction(this, tableMetadata, session, false, tableLocation, _ -> false, GC_ENABLED_DEFAULT);

Transaction transaction = IcebergUtil.newCreateTableTransaction(this, tableMetadata, session, false, tableLocation, _ -> false, true);

findinpath · 2025-11-19T09:20:46Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java

    }

-    public static Transaction newCreateTableTransaction(TrinoCatalog catalog, ConnectorTableMetadata tableMetadata, ConnectorSession session, boolean replace, String tableLocation, Predicate<String> allowedExtraProperties)
+    public static Transaction newCreateTableTransaction(TrinoCatalog catalog, ConnectorTableMetadata tableMetadata, ConnectorSession session, boolean replace, String tableLocation, Predicate<String> allowedExtraProperties, boolean defaultNewTablesGcEnabled)


nit: Please separate this long list of parameters on new lines for increased readability.

findinpath · 2025-11-19T09:21:50Z

...n/trino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/glue/TestTrinoGlueCatalog.java

                    newDirectExecutorService(),
-                    newDirectExecutorService());
+                    newDirectExecutorService(),
+                    GC_ENABLED_DEFAULT);


Suggested change

GC_ENABLED_DEFAULT);

true);

findinpath · 2025-11-19T09:22:00Z

...ino-iceberg/src/test/java/io/trino/plugin/iceberg/catalog/nessie/TestTrinoNessieCatalog.java

                    newDirectExecutorService(),
-                    newDirectExecutorService());
+                    newDirectExecutorService(),
+                    GC_ENABLED_DEFAULT);


Suggested change

GC_ENABLED_DEFAULT);

true);

findinpath · 2025-11-19T09:33:08Z

iceberg.default-new-tables-gc.enabled will set gc.enabled flag for new tables created by Trino.

Why should the Trino Iceberg connector be proactive in setting the gc.enabled field on the tables created?

Assuming that we find a good rationale on the question above, I suggest rather to handle the implementation of the config property rather differently.
Take hive.max-splits-per-second as a proof of concept


    @Nullable
    public Boolean getDefaultGarbageCollection()
    {
        return defaultGarbageCollection;
    }

    @Config("iceberg.default-garbage-collection")
    @ConfigDescription("Default value for Iceberg property gc.enabled when creating new tables")
    public IcebergConfig setDefaultGarbageCollection(Boolean defaultGarbageCollection)
    {
        this.defaultGarbageCollection = defaultGarbageCollection;
        return this;
    }

iceberg.object-store-layout.enabled - although applies for new tables, does not have new-table token in the property name.

iceberg.default-garbage-collection can be true, false or not specified.

sopel39 · 2025-11-19T14:03:53Z

Why should the Trino Iceberg connector be proactive in setting the gc.enabled field on the tables created?

It's not proactive. It only sets it when the config toggle is different than GC_ENABLED

chenjian2664 · 2025-11-19T14:28:22Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

+        assertThat(ctasMetadata.properties().getOrDefault("gc.enabled", "true")).isEqualTo("true");
+
+        // collect data files and verify they are removed on drop
+        var ctasDataFiles = getAllDataFilesFromTableDirectory(ctasTable);


Can we avoid to use var ?

chenjian2664 · 2025-11-19T14:28:44Z

plugin/trino-iceberg/src/test/java/io/trino/plugin/iceberg/BaseIcebergConnectorTest.java

+        String ctasTableLocation = getTableLocation(ctasTable);
+        String ctasMetadataLocation = getLatestMetadataLocation(fileSystem, ctasTableLocation);
+        TableMetadata ctasMetadata = TableMetadataParser.read(FILE_IO_FACTORY.create(fileSystem), ctasMetadataLocation);
+        assertThat(ctasMetadata.properties().getOrDefault("gc.enabled", "")).isEqualTo("");


use isEmpty()

chenjian2664 · 2025-11-19T14:35:45Z

plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/IcebergUtil.java

+
+        // If user doesn't set gc.enabled, we need to set it to defaultNewTablesGcEnabled value
+        if (!extraProperties.containsKey(GC_ENABLED) && defaultNewTablesGcEnabled != GC_ENABLED_DEFAULT) {
+            propertiesBuilder.put(GC_ENABLED, Boolean.toString(defaultNewTablesGcEnabled));


It looks like a breaking change to expire_snapshots?

But if other engine disable the gc flag, it's also a problem to Trino, user not able to expire snapshots due a unsupported or exposed property

sopel39 · 2025-11-19T14:50:14Z

After discussion with @raunaqmorarka and @ebyhr we figured that gc.enabled might not be the right approach, because it prevents both data delete on table drop and maintainable ops (e.g. expire snapshots).

Alternatives are:

Trino specific property that prevents data delete on table drop in Trino only, but allows maintenance operations.
Iceberg native property that prevents data delete on table drop across all engines, but allows maintenance operations.

sopel39 requested review from ebyhr and raunaqmorarka November 18, 2025 14:39

cla-bot bot added the cla-signed label Nov 18, 2025

github-actions bot added the iceberg Iceberg connector label Nov 18, 2025

sopel39 force-pushed the oss/default_new_tables_gc_enabled branch from 9b10773 to c52b655 Compare November 18, 2025 15:13

Add iceberg.default-new-tables-gc.enabled config property

94dcc5d

iceberg.default-new-tables-gc.enabled will set gc.enabled flag for new tables created by Trino.

sopel39 force-pushed the oss/default_new_tables_gc_enabled branch from c52b655 to 94dcc5d Compare November 18, 2025 15:59

raunaqmorarka reviewed Nov 18, 2025

View reviewed changes

ebyhr reviewed Nov 19, 2025

View reviewed changes

raunaqmorarka requested review from chenjian2664, findepi, findinpath and hashhar November 19, 2025 04:31

findinpath reviewed Nov 19, 2025

View reviewed changes

chenjian2664 reviewed Nov 19, 2025

View reviewed changes

	if (propertyAsBoolean(metadata.properties(), GC_ENABLED, GC_ENABLED_DEFAULT)) {
	if (propertyAsBoolean(metadata.properties(), GC_ENABLED, true)) {

	if (propertyAsBoolean(table.properties(), GC_ENABLED, GC_ENABLED_DEFAULT)) {
	if (propertyAsBoolean(table.properties(), GC_ENABLED, true)) {

	Map<String, String> properties = createTableProperties(new ConnectorTableMetadata(storageTableName, columns, materializedViewProperties, Optional.empty()), _ -> false, GC_ENABLED_DEFAULT);
	Map<String, String> properties = createTableProperties(new ConnectorTableMetadata(storageTableName, columns, materializedViewProperties, Optional.empty()), _ -> false, true);

	Transaction transaction = IcebergUtil.newCreateTableTransaction(this, tableMetadata, session, false, tableLocation, _ -> false, GC_ENABLED_DEFAULT);
	Transaction transaction = IcebergUtil.newCreateTableTransaction(this, tableMetadata, session, false, tableLocation, _ -> false, true);

Add iceberg.default-new-tables-gc.enabled config property #27359

Are you sure you want to change the base?

Add iceberg.default-new-tables-gc.enabled config property #27359

Uh oh!

Conversation

sopel39 commented Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Additional context and related issues

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopel39 Nov 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findinpath Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

findinpath commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sopel39 commented Nov 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sopel39 commented Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

sopel39 commented Nov 18, 2025 •

edited

Loading

sopel39 Nov 18, 2025 •

edited

Loading

findinpath Nov 19, 2025 •

edited

Loading

findinpath commented Nov 19, 2025 •

edited

Loading

sopel39 commented Nov 19, 2025 •

edited

Loading