Analyze Iceberg tables #13636

findepi · 2022-08-12T09:34:37Z

Support ANALYZE in Iceberg connector. This collects number distinct
values (NDV) of selected columns and stores that in table properties.
This is interim solution until Iceberg library has first-class
statistics files support.

alexjo2144 · 2022-08-12T18:30:17Z

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

+    public ConnectorAnalyzeMetadata getStatisticsCollectionMetadata(ConnectorSession session, ConnectorTableHandle tableHandle, Map<String, Object> analyzeProperties)
+    {
+        IcebergTableHandle handle = (IcebergTableHandle) tableHandle;
+        checkArgument(handle.getTableType() == DATA, "Cannot analyze non-DATA table: %s", handle.getTableType());


Should getTableHandleForExecute have a similar check?

Also, do we verify that the table's snapshot is the recent one? Not time traveling

alexjo2144 · 2022-08-12T18:34:16Z

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

+
+        Set<ColumnStatisticMetadata> columnStatistics = tableMetadata.getColumns().stream()
+                .filter(column -> !column.isHidden())
+                .filter(column -> analyzeColumnNames.contains(column.getName()))


Do you need to lowercase analyzeColumnNames ?

The user needs to provide them in lowercase. This makes it easier to support non-lowercase column names in the future.

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

findepi · 2022-08-24T09:48:47Z

@alexjo2144 thanks for your review.
AC. Had to rebase due to a conflict.

findepi · 2022-08-24T10:01:11Z

Prefix extracted: #13823

findepi · 2022-08-25T07:46:03Z

Prefix extracted: #13823

rebasing after that one merged. Was green before (https://github.com/trinodb/trino/runs/8002212012?check_suite_focus=true)

losipiuk · 2022-08-25T08:24:33Z

core/trino-main/src/main/java/io/trino/testing/TestingAccessControlManager.java

+        if (shouldDenyPrivilege(context.getIdentity().getUser(), table + "." + procedure, EXECUTE_TABLE_PROCEDURE)) {
+            denyExecuteTableProcedure(table.toString(), procedure);
+        }
+        if (denyPrivileges.isEmpty()) {


why this if?

I am dumbly following the pattern in this class.

losipiuk · 2022-08-25T08:28:06Z

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

@@ -917,6 +933,18 @@ private Optional<ConnectorTableExecuteHandle> getTableHandleForOptimize(Connecto
                icebergTable.location()));
    }

+    private Optional<ConnectorTableExecuteHandle> getTableHandleForDropExtendedStats(ConnectorSession session, IcebergTableHandle tableHandle, Map<String, Object> executeProperties)
+    {
+        checkProcedureArgument(executeProperties.isEmpty(), "Unexpected properties");


nit: quote properties in error message?

will do. but that's just some check and the proper checks are done in the engine.
in fact, see getTableHandleForOptimize. it gets a propery value from the Map, but doesn't check that the map doesn't have extra entries.

so, to keep code simpler, will just drop the check. this will prevent someone in the future to be forced to do solid validation here

losipiuk · 2022-08-25T10:12:45Z

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

+                .orElse(allColumnNames);
+
+        Set<ColumnStatisticMetadata> columnStatistics = tableMetadata.getColumns().stream()
+                .filter(column -> !column.isHidden())


nit: looks like filtering out hidden is not needed here as you enforce that analyzeColumnNames not contains hidden columns above.

removed and renamed allColumnNames to allDataColumnNames.

losipiuk · 2022-08-25T10:25:14Z

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

@@ -2847,6 +2847,47 @@ public void testBasicTableStatistics()
        dropTable(tableName);
    }

+    @Test
+    public void testBasicAnalyze()


nit: you can also test if stats go away if you explicitly drop them.

This is covered by io.trino.plugin.iceberg.TestIcebergAnalyze#testDropExtendedStats

electrum

We shouldn’t add this temporary feature. We’ll have to support it forever on the read side since users will rely on it, as the stats will be stored permanently in their tables and dropping it will break their queries.

What is the migration plan for the new implementation?

findepi · 2022-08-26T10:50:56Z

What is the migration plan for the new implementation?

discussing over slack

findepi · 2022-08-30T13:52:11Z

(just rebased)

findepi · 2022-08-30T18:02:48Z

CI #12385 & sth other

ebyhr · 2022-08-31T07:06:33Z

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

+        String tableName = "test_analyze";
+        assertUpdate("CREATE TABLE " + tableName + " AS SELECT * FROM tpch.tiny.region", 5);
+
+                        // no NDV information


nit: The indentation seems wrong. Please ignore if it's intentional.

thanks, fixed

findepi · 2022-09-06T09:14:12Z

We shouldn’t add this temporary feature. We’ll have to support it forever on the read side since users will rely on it, as the stats will be stored permanently in their tables and dropping it will break their queries.

Discussed offline and there were no objections overall.

I will still add a toggle to make this an opt-in, experimental feature so that we can accelerate deprecation path.

findepi · 2022-09-06T09:14:41Z

(just rebased)

Per code style rule

Support `ANALYZE` in Iceberg connector. This collects number distinct values (NDV) of selected columns and stores that in table properties. This is interim solution until Iceberg library has first-class statistics files support.

findepi · 2022-09-07T11:37:08Z

No release notes because this is experimental.

zinking · 2022-10-27T08:29:33Z

@findepi where is the ndv generated ? from this PR it seems it's already in the table properties. how does it work currently?

findepi added enhancement New feature or request performance labels Aug 12, 2022

findepi requested review from electrum, homar, ebyhr, alexjo2144, hashhar and findinpath August 12, 2022 09:34

cla-bot bot added the cla-signed label Aug 12, 2022

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch 3 times, most recently from 427d7f5 to 92d8312 Compare August 12, 2022 09:41

findepi mentioned this pull request Aug 12, 2022

Analyze Iceberg tables #12317

Closed

findepi self-assigned this Aug 12, 2022

github-actions bot added the tests:hive label Aug 12, 2022

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from 92d8312 to e1d1c7d Compare August 12, 2022 16:22

alexjo2144 reviewed Aug 12, 2022

View reviewed changes

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from e1d1c7d to b47566d Compare August 24, 2022 09:48

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from b47566d to b3efc19 Compare August 24, 2022 09:52

findepi requested review from losipiuk and alexjo2144 and removed request for alexjo2144 August 24, 2022 09:52

findepi mentioned this pull request Aug 24, 2022

Iceberg test improvements around time travel #13823

Merged

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch 2 times, most recently from b7bb888 to 60f88fb Compare August 24, 2022 18:59

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from 60f88fb to 5e9f939 Compare August 25, 2022 07:46

losipiuk reviewed Aug 25, 2022

View reviewed changes

losipiuk approved these changes Aug 25, 2022

View reviewed changes

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch 2 times, most recently from 60598bb to d5870f7 Compare August 25, 2022 11:39

electrum previously requested changes Aug 25, 2022

View reviewed changes

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from d5870f7 to 5c5c16c Compare August 26, 2022 10:23

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from 5c5c16c to 474b632 Compare August 30, 2022 13:52

ebyhr approved these changes Aug 31, 2022

View reviewed changes

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from d9af33c to f4b0fd4 Compare August 31, 2022 10:40

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from f4b0fd4 to cb74fab Compare September 6, 2022 09:14

Put getter before setter

85ec919

Per code style rule

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch 2 times, most recently from 3655c82 to 95ccfb6 Compare September 6, 2022 10:51

Analyze Iceberg tables

0901525

Support `ANALYZE` in Iceberg connector. This collects number distinct values (NDV) of selected columns and stores that in table properties. This is interim solution until Iceberg library has first-class statistics files support.

findepi force-pushed the findepi/iceberg-analyze-ndv-sans-stats-file branch from 95ccfb6 to 0901525 Compare September 6, 2022 14:03

findepi merged commit da150ae into trinodb:master Sep 7, 2022

findepi deleted the findepi/iceberg-analyze-ndv-sans-stats-file branch September 7, 2022 11:36

findepi added the no-release-notes This pull request does not require release notes entry label Sep 7, 2022

github-actions bot added this to the 395 milestone Sep 7, 2022

colebow mentioned this pull request Sep 7, 2022

Add Trino 395 release notes #13975

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Analyze Iceberg tables #13636

Analyze Iceberg tables #13636

findepi commented Aug 12, 2022

alexjo2144 Aug 12, 2022

alexjo2144 Aug 12, 2022

findepi Aug 24, 2022

alexjo2144 Aug 12, 2022

findepi Aug 24, 2022

findepi commented Aug 24, 2022

findepi commented Aug 24, 2022

findepi commented Aug 25, 2022

losipiuk Aug 25, 2022

findepi Aug 25, 2022

losipiuk Aug 25, 2022

findepi Aug 25, 2022

losipiuk Aug 25, 2022

findepi Aug 25, 2022

findepi Aug 25, 2022

losipiuk Aug 25, 2022

findepi Aug 25, 2022

electrum left a comment

findepi commented Aug 26, 2022

findepi commented Aug 30, 2022

findepi commented Aug 30, 2022

ebyhr Aug 31, 2022

findepi Aug 31, 2022

findepi commented Sep 6, 2022

findepi commented Sep 6, 2022

findepi commented Sep 7, 2022

zinking commented Oct 27, 2022

Analyze Iceberg tables #13636

Analyze Iceberg tables #13636

Conversation

findepi commented Aug 12, 2022

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

findepi commented Aug 24, 2022

findepi commented Aug 24, 2022

findepi commented Aug 25, 2022

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

electrum left a comment

Choose a reason for hiding this comment

findepi commented Aug 26, 2022

findepi commented Aug 30, 2022

findepi commented Aug 30, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Sep 6, 2022

findepi commented Sep 6, 2022

findepi commented Sep 7, 2022

zinking commented Oct 27, 2022