Fix support for system tables for Lakehouse #26830

codluca · 2025-10-03T09:13:20Z

Fix support for system tables for Lakehouse.
The $files and $history system tables for Iceberg tables are not handled properly.

Fixes #26751

Description

For system tables, the handle will be an instance of io.trino.connector.system.SystemTableHandle.
Some Iceberg system tables ($files and $history) use special splits (FilesTableSplit).

io.trino.connector.system.SystemTableHandle is part of trino-main, which is a test dependency for trino-lakehouse.
The dependency to trino-main is not changed.

Release notes

## Lakehouse
* Fix failure when reading metadata tables. ({issue}`26751`)

ebyhr · 2025-10-03T23:04:41Z

...-lakehouse/src/test/java/io/trino/plugin/lakehouse/TestLakehouseDeltaConnectorSmokeTest.java

    }
+
+    @Test
+    public void testSelectDeltaMetaTables()


https://trino.io/docs/current/develop/tests.html#conventions-and-recommendations

Test methods should be defined as package-private.

This test can be renamed to testSelectMetadataTable.

"Test classes should be defined as package-private and final."
"Test methods should be defined as package-private."
Should I change the class to be package private and final, and the other methods to be package private?
Or just the new methods?

You can change existing visibility, but I recommend handling in a separate PR.

ebyhr · 2025-10-03T23:05:32Z

...-lakehouse/src/test/java/io/trino/plugin/lakehouse/TestLakehouseDeltaConnectorSmokeTest.java

+    {
+        computeActual(
+                """
+                CREATE TABLE delta_table


Please avoid a static table name. Use TestTable with try-with-resources instead. Or please consider using the existing table.

Used existing table

ebyhr · 2025-10-03T23:05:52Z

...-lakehouse/src/test/java/io/trino/plugin/lakehouse/TestLakehouseDeltaConnectorSmokeTest.java

+                AS SELECT * FROM tpch.tiny.region
+                """);
+
+        assertQuery("SELECT count(*) FROM \"delta_table$history\"", "VALUES 1");


assertQuery method isn't recommend. Please use assertThat(query(...)) instead.

Changed to using assertThat(query(...))

ebyhr · 2025-10-03T23:08:46Z

...-lakehouse/src/test/java/io/trino/plugin/lakehouse/TestLakehouseDeltaConnectorSmokeTest.java

+    @Test
+    public void testSelectDeltaMetaTables()
+    {
+        computeActual(


We usually use assertUpdate for DDL.

Changed to use existing table

chenjian2664 · 2025-10-04T10:03:00Z

...no-lakehouse/src/main/java/io/trino/plugin/lakehouse/LakehousePageSourceProviderFactory.java

+            default -> {
+                // For system tables, the handle will be an instance of io.trino.connector.system.SystemTableHandle
+                // Some Iceberg system tables ($files and $history) use special splits (FilesTableSplit)
+                if (split instanceof FilesTableSplit) {


What's the handle in this case? it should be the FilesTable right?

No, it's of io.trino.connector.system.SystemTableHandle type.
And the split is of io.trino.plugin.iceberg.system.files.FilesTableSplit type.

io.trino.connector.system.SystemTableHandle is from trino-main module, which is only a test dependency. That's why I didn't add a new switch case for SystemTableHandle.

Thanks.
In this case we could add a case branch instead of putting it in the default, and raise nice error message to mention which kind of split we are not support yet with the SystemTableHandle

io.trino.connector.system.SystemTableHandle is from trino-main module, which is only a test dependency.
What should be the new scope of the trino-main module?

Make that scope to compile should be fine in this case

Disagree. Please don't include trino-main as a dependency. The module isn't supposed to be depended on by connectors.

It wouldn't work anyway, as the class is in a different class loader.

Thanks @ebyhr @electrum , just got to know

ebyhr · 2025-10-04T22:36:33Z

...-lakehouse/src/test/java/io/trino/plugin/lakehouse/TestLakehouseDeltaConnectorSmokeTest.java

+        assertThatThrownBy(() -> computeScalar("SELECT count(*) FROM lakehouse.tpch.\"region$files\""))
+                .hasMessageMatching(".* Table .* does not exist");
+        assertThatThrownBy(() -> computeScalar("SELECT count(*) FROM lakehouse.tpch.\"region$timeline\""))
+                .hasMessageMatching(".* Table .* does not exist");


Please use assertThat(query(...)).failure() so we can ensure that the exception class is TrinoException.

I'm not able to get the assertion to work. I can't find an example either.
TrinoException is wrapped in a QueryFailedException, which has a cause of FailureException.
The TrinoException is stored as FailureInfo in FailureException.

assertThat(query("SELECT count(*) FROM lakehouse.tpch.\"region$history\"")) .failure().isInstanceOf(TrinoException.class).hasMessage(".* Table .* does not exist"); - fails with -> java.lang.AssertionError: Expecting actual throwable to be an instance of: io.trino.spi.TrinoException but was: io.trino.testing.QueryFailedException: line 1:22: Table 'lakehouse.tpch."region$history"' does not exist

assertThat(query("SELECT count(*) FROM lakehouse.tpch.\"region$history\"")) .failure().hasCauseInstanceOf(TrinoException.class).hasMessage(".* Table .* does not exist"); - fails with -> java.lang.AssertionError: Expecting a throwable with cause being an instance of: io.trino.spi.TrinoException but was an instance of: io.trino.client.FailureException Throwable that failed the check: io.trino.testing.QueryFailedException: line 1:22: Table 'lakehouse.tpch."region$history"' does not exist assertThatThrownBy(() -> computeScalar("SELECT count(*) FROM lakehouse.tpch.\"region$history\"")) .isInstanceOf(TrinoException.class).hasMessageMatching(".* Table .* does not exist";

assertThatThrownBy(() -> computeScalar("SELECT count(*) FROM lakehouse.tpch.\"region$history\"")) .isInstanceOf(TrinoException.class).hasMessageMatching(".* Table .* does not exist";

fails like the first example above

For system tables, the handle will be an instance of io.trino.connector.system.SystemTableHandle. Some Iceberg system tables ($files and $history) use special splits (FilesTableSplit).

cla-bot bot added the cla-signed label Oct 3, 2025

github-actions bot added the lakehouse label Oct 3, 2025

wendigo requested a review from chenjian2664 October 3, 2025 09:14

ebyhr self-requested a review October 3, 2025 09:21

codluca force-pushed the 26751-lakehouse-system-tables branch from 7356018 to 3ad2a43 Compare October 3, 2025 16:09

github-actions bot added the iceberg Iceberg connector label Oct 3, 2025

codluca force-pushed the 26751-lakehouse-system-tables branch from 3b04a7b to 07fce46 Compare October 3, 2025 17:50

martint requested a review from electrum October 3, 2025 17:52

ebyhr reviewed Oct 3, 2025

View reviewed changes

chenjian2664 reviewed Oct 4, 2025

View reviewed changes

codluca force-pushed the 26751-lakehouse-system-tables branch from 2aeb63e to 10fecac Compare October 4, 2025 13:11

ebyhr reviewed Oct 4, 2025

View reviewed changes

codluca force-pushed the 26751-lakehouse-system-tables branch from 8376952 to 7eee45e Compare October 5, 2025 19:23

Fix support for system tables for Lakehouse

a83442b

For system tables, the handle will be an instance of io.trino.connector.system.SystemTableHandle. Some Iceberg system tables ($files and $history) use special splits (FilesTableSplit).

codluca force-pushed the 26751-lakehouse-system-tables branch from 7eee45e to a83442b Compare October 7, 2025 12:14

Fix support for system tables for Lakehouse #26830

Are you sure you want to change the base?

Fix support for system tables for Lakehouse #26830

Uh oh!

Conversation

codluca commented Oct 3, 2025 • edited by ebyhr Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Release notes

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!

ebyhr Oct 3, 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!

codluca Oct 4, 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!

chenjian2664 Oct 12, 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!

ebyhr Oct 12, 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!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

codluca commented Oct 3, 2025 •

edited by ebyhr

Loading

ebyhr Oct 3, 2025 •

edited

Loading

codluca Oct 4, 2025 •

edited

Loading

chenjian2664 Oct 12, 2025 •

edited

Loading

ebyhr Oct 12, 2025 •

edited

Loading