Make Iceberg's procedures uniformly support named arguments by hantangwangd · Pull Request #23592 · prestodb/presto

hantangwangd · 2024-09-05T16:22:56Z

Description

This PR enables existing procedures which do not yet support named arguments to support named arguments, ensures consistency in the naming of arguments across procedures, and supplements the document with explanations for named arguments and sequential arguments.

Besides, this PR removes a procedure set_table_property which is completely unused.

Motivation and Context

Make Iceberg's procedures uniformly support named arguments

Impact

After this change, Iceberg procedures register_table and unregister_table support named arguments as well

Test Plan

Newly added test cases to show the support of using named arguments in procedure register_table and unregister_table

Contributor checklist

Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
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
* Support using named arguments in procedure `register_table` and `unregister_table` :pr:`12345`

steveburnett

Thanks for the doc! A formatting nit and a suggested rephrasing to consider.

presto-docs/src/main/sphinx/connector/iceberg.rst

ZacBlanco

Thank you for these changes! I appreciate consistency 😄

ZacBlanco · 2024-09-05T18:30:41Z

...o-iceberg/src/main/java/com/facebook/presto/iceberg/procedure/SetTablePropertyProcedure.java

-                .set(key, value)
-                .commit();
-    }
-}


Is there a good reason to remove it? If I recall, the reason I added this was because iceberg's table properties are generic string-string key-value pairs. Presto doesn't allow setting any arbitrary properties through regular SQL syntax though. So this adds some useful functionality for users who might have some arbitrary table-level metadata to store.

Besides, this PR removes a procedure set_table_property which is completely unused.

@hantangwangd It was unused as it's missing binding in IcebergCommonModule, so the Procedure wasn't even enabled till now.

We can add the binding and enable it as it should be useful as Zac mentioned. What do you think?

I just saw that this procedure was not used at all, not even registered as @imjalpreet mentioned. I do not have a strong feeling to remove it, since the PR #21495 has been blocked for quite some time, I would like to register and enable this procedure if you think it's necessary. Let me know your thoughts @ZacBlanco @imjalpreet.

Now keep SetTablePropertyProcedure as it is, maybe it's better to handle it in a later dedicated PR.

Sounds good, we can enable the procedure and add some tests in a follow-up PR.

presto-docs/src/main/sphinx/connector/iceberg.rst

presto-iceberg/src/main/java/com/facebook/presto/iceberg/procedure/RegisterTableProcedure.java

agrawalreetika · 2024-09-06T09:34:01Z

...ceberg/src/test/java/com/facebook/presto/iceberg/procedure/TestIcebergRegisterProcedure.java

+        assertUpdate("CALL system.unregister_table(table_name => '" + tableName + "', schema => '" + TEST_SCHEMA + "')");
+        assertFalse(getQueryRunner().tableExists(getSession(), tableName));
+    }
+


Would it be better to rename either the this test class or Add a new Test class to incorporate unregister_table procedure related tests?

Also test classes for SetTablePropertyProcedure & RollbackToSnapshotProcedure procedures are missing, should we add few tests for that as well Or may be handle tests in future PR? WDYT?

Would it be better to rename either the this test class or Add a new Test class to incorporate unregister_table procedure related tests?

Good suggestion, I have renamed the test class.

Also test classes for SetTablePropertyProcedure & RollbackToSnapshotProcedure procedures are missing, should we add few tests for that as well Or may be handle tests in future PR?

Under your reminder, I realized that there are still some different kinds of work to be done. I think it's better to leave the work of registering and documenting SetTablePropertyProcedure, adding test cases for both SetTablePropertyProcedure and RollbackToSnapshotProcedure to subsequent separated PRs. And in this PR, we focus on support named arguments uniformly for existing procedures, and unifying the naming conventions for their arguments. Do you think this makes sense?

Sounds good to me 👍🏻

steveburnett

LGTM! (docs)

Pull updated branch, new local docs build, looks good. Thanks!

imjalpreet

LGTM, thank you.

steveburnett

LGTM! (docs)

Pull updated branch, new local docs build, looks good. Thanks!

hantangwangd requested review from a team, ZacBlanco, elharo and steveburnett as code owners September 5, 2024 16:22

hantangwangd requested a review from presto-oss September 5, 2024 16:22

tdcmeehan self-assigned this Sep 5, 2024

steveburnett requested changes Sep 5, 2024

View reviewed changes

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

presto-docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved

hantangwangd force-pushed the standardize_iceberg_procedures branch from 0062546 to 7bd5797 Compare September 5, 2024 17:55

ZacBlanco requested changes Sep 5, 2024

View reviewed changes

imjalpreet reviewed Sep 5, 2024

View reviewed changes

presto-iceberg/src/main/java/com/facebook/presto/iceberg/procedure/RegisterTableProcedure.java Outdated Show resolved Hide resolved

hantangwangd force-pushed the standardize_iceberg_procedures branch from 7bd5797 to b496ec0 Compare September 6, 2024 01:39

agrawalreetika reviewed Sep 6, 2024

View reviewed changes

steveburnett previously approved these changes Sep 6, 2024

View reviewed changes

hantangwangd added 2 commits September 7, 2024 02:34

[Doc]Supplement explanations on named and positional arguments in CALL

dcb12c5

Fix Iceberg procedures to uniformly support named arguments

b54137e

hantangwangd dismissed steveburnett’s stale review via b54137e September 6, 2024 18:36

hantangwangd force-pushed the standardize_iceberg_procedures branch from b496ec0 to b54137e Compare September 6, 2024 18:36

ZacBlanco approved these changes Sep 6, 2024

View reviewed changes

agrawalreetika approved these changes Sep 6, 2024

View reviewed changes

imjalpreet approved these changes Sep 9, 2024

View reviewed changes

steveburnett approved these changes Sep 9, 2024

View reviewed changes

hantangwangd merged commit 1a2228d into prestodb:master Sep 9, 2024

hantangwangd deleted the standardize_iceberg_procedures branch September 9, 2024 23:21

jaystarshot mentioned this pull request Nov 1, 2024

Add release notes for 0.290 #23936

Merged

25 tasks

Comments

Conversation

hantangwangd commented Sep 5, 2024

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ZacBlanco left a comment

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!

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!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

imjalpreet left a comment

Choose a reason for hiding this comment

Uh oh!

steveburnett left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants