Procedure to compute table stats by karuppayya · Pull Request #10986 · apache/iceberg

karuppayya · 2024-08-22T05:04:29Z

No description provided.

karuppayya · 2024-08-22T05:07:04Z

@aokolnychyi @szehon-ho Can you help review this please

nastra · 2024-08-22T14:00:25Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

+    sql("CREATE TABLE %s (id bigint NOT NULL, data string) USING iceberg", tableName);
+    List<Object[]> result =
+        sql("CALL %s.system.compute_table_stats('%s')", catalogName, tableIdent);
+    Assertions.assertTrue(result.isEmpty());


Suggested change

Assertions.assertTrue(result.isEmpty());

assertThat(result).isEmpty();

please use the AssertJ assertions and we generally try to avoid ussing assertTrue/assertFalse as it doesn't provide enough context about the actual/expected when an assertion fails

nastra · 2024-08-22T14:01:15Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

+        sql(
+            "CALL %s.system.compute_table_stats(table => '%s', columns => array('id'))",
+            catalogName, tableIdent);
+    Assertions.assertNotNull(output.get(0));


Suggested change

Assertions.assertNotNull(output.get(0));

assertThat(output.get(0)).isNotNull();

should this maybe also assert some more details here?

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

nastra · 2024-08-22T14:03:38Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

+            IllegalArgumentException.class,
+            () ->
+                sql(
+                    "CALL %s.system.compute_table_stats(table => '%s', snapshot_id => %dL)",


maybe also add a test with an invalid/non-existing table name

@karuppayya can you please add a test with an invalid/non-existing table?

Added this test.

nastra · 2024-08-22T14:04:43Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

+        sql(
+            "CALL %s.system.compute_table_stats('%s', %dL)",
+            catalogName, tableIdent, snapshot.snapshotId());
+    Assertions.assertNotNull(output.get(0));


same as above

aokolnychyi · 2024-08-22T19:48:22Z

I will take a look at the partition stats PR first by @ajantha-bhat. I want to understand if we want a single analyze procedure or different procedures for table and partition stats.

nastra · 2024-08-23T05:03:31Z

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

+        sql(
+            "CALL %s.system.compute_table_stats('%s', %dL)",
+            catalogName, tableIdent, snapshot.snapshotId());
+    assertThat(output.get(0)).isNotNull();


I think it would be good to check whether the output has some valid results here and in the other test

nastra

LGTM, but I left a few comments around testing that would be good to address

karuppayya · 2024-10-28T18:07:10Z

@aokolnychyi Can this merged now?

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/ComputeTableStatsProcedure.java

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

(cherry picked from commit 49e668e687e0a5d77652405ba14bdc3ee592f261)

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java

szehon-ho · 2024-11-19T23:20:44Z

This looks good to me, will merge tomorrow if no additional comments

ajantha-bhat · 2024-11-20T06:58:39Z

I will take a look at the partition stats PR first by @ajantha-bhat. I want to understand if we want a single analyze procedure or different procedures for table and partition stats.

I think single analyze procedure is fine. We can merge this PR. I will do a follow up to accept list of ENUMs to compute the required type of stats (default compute everything). We need to also modify spark action to compute partition stats.

ajantha-bhat · 2024-11-20T07:00:11Z

spark/v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/SparkProcedures.java

    mapBuilder.put("create_changelog_view", CreateChangelogViewProcedure::builder);
    mapBuilder.put("rewrite_position_delete_files", RewritePositionDeleteFilesProcedure::builder);
    mapBuilder.put("fast_forward", FastForwardBranchProcedure::builder);
+    mapBuilder.put("compute_table_stats", ComputeTableStatsProcedure::builder);


Please also document it under
https://github.com/apache/iceberg/blob/main/docs/docs/spark-procedures.md

Thanks @ajantha-bhat ,
I will create a follow up PR for the doc changes, just to keep this PR focussed on the procedure.

aokolnychyi · 2024-11-20T20:59:20Z

I'd love to review today as well.

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/ComputeTableStatsProcedure.java

aokolnychyi

Minor styling suggestions. LGTM.

szehon-ho · 2024-11-20T23:11:02Z

Looks like all comment addressed, merged, can do a follow up if more. Thanks @karuppayya , and also @aokolnychyi @ajantha-bhat @nastra for addition reviews!

github-actions bot added the spark label Aug 22, 2024

nastra reviewed Aug 22, 2024

View reviewed changes

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java Outdated Show resolved Hide resolved

nastra reviewed Aug 22, 2024

View reviewed changes

karuppayya force-pushed the compute_stats_procedure branch from de6f331 to 2b6e107 Compare August 22, 2024 22:35

nastra reviewed Aug 23, 2024

View reviewed changes

nastra approved these changes Aug 23, 2024

View reviewed changes

karuppayya force-pushed the compute_stats_procedure branch from 2b6e107 to c2790a8 Compare August 30, 2024 00:07

karuppayya mentioned this pull request Sep 19, 2024

Spark Action to Analyze table #10288

Merged

szehon-ho reviewed Oct 31, 2024

View reviewed changes

karuppayya force-pushed the compute_stats_procedure branch from c2790a8 to 1cb1ad0 Compare November 11, 2024 22:23

szehon-ho approved these changes Nov 15, 2024

View reviewed changes

krajendran4 added 5 commits November 18, 2024 13:59

compute table stats procedure

53e403e

(cherry picked from commit 49e668e687e0a5d77652405ba14bdc3ee592f261)

Address review comments

f8e8b5a

Address review comments

2cccc2f

Address review comments

9554114

Address review comments

62c826f

karuppayya force-pushed the compute_stats_procedure branch from 5def103 to 62c826f Compare November 18, 2024 21:59

nastra reviewed Nov 19, 2024

View reviewed changes

...nsions/src/test/java/org/apache/iceberg/spark/extensions/TestComputeTableStatsProcedure.java Outdated Show resolved Hide resolved

krajendran4 added 2 commits November 19, 2024 11:54

Address review comment

3cd25df

Fix test

1a42a14

ajantha-bhat reviewed Nov 20, 2024

View reviewed changes

Address review comments

490847f

aokolnychyi reviewed Nov 20, 2024

View reviewed changes

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/ComputeTableStatsProcedure.java Outdated Show resolved Hide resolved

aokolnychyi reviewed Nov 20, 2024

View reviewed changes

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/ComputeTableStatsProcedure.java Outdated Show resolved Hide resolved

aokolnychyi reviewed Nov 20, 2024

View reviewed changes

...v3.5/spark/src/main/java/org/apache/iceberg/spark/procedures/ComputeTableStatsProcedure.java Show resolved Hide resolved

aokolnychyi approved these changes Nov 20, 2024

View reviewed changes

Address review comments

120e7f8

szehon-ho merged commit 3b5c9f7 into apache:main Nov 20, 2024

nastra pushed a commit to nastra/iceberg that referenced this pull request Nov 21, 2024

Spark 3.5: Procedure to compute table stats (apache#10986)

05172c9

jeesou mentioned this pull request Nov 26, 2024

Adding ComputeTableStats Procedure to Spark 3.4 #11652

Merged

zachdisc pushed a commit to zachdisc/iceberg that referenced this pull request Dec 23, 2024

Spark 3.5: Procedure to compute table stats (apache#10986)

7c25b27

karuppayya mentioned this pull request May 20, 2025

Spark-3.5: Add spark action to compute partition stats #12450

Merged

	Assertions.assertTrue(result.isEmpty());
	assertThat(result).isEmpty();

	Assertions.assertNotNull(output.get(0));
	assertThat(output.get(0)).isNotNull();

Conversation

karuppayya commented Aug 22, 2024

Uh oh!

karuppayya commented Aug 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

aokolnychyi commented Aug 22, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nastra left a comment

Choose a reason for hiding this comment

Uh oh!

karuppayya commented Oct 28, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

szehon-ho commented Nov 19, 2024

Uh oh!

ajantha-bhat commented Nov 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aokolnychyi commented Nov 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aokolnychyi left a comment

Choose a reason for hiding this comment

Uh oh!

szehon-ho commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

szehon-ho commented Nov 20, 2024 •

edited

Loading