Skip to content

Make Iceberg's procedures uniformly support named arguments #23592

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
hantangwangd merged 2 commits into from
Sep 9, 2024
Merged

Make Iceberg's procedures uniformly support named arguments #23592

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dcb12c5
[Doc]Supplement explanations on named and positional arguments in CALL
hantangwangd Sep 6, 2024
b54137e
Fix Iceberg procedures to uniformly support named arguments
hantangwangd Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 41 additions & 6 deletions presto-docs/src/main/sphinx/connector/iceberg.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,28 +732,49 @@ example uses the earliest snapshot ID: ``2423571386296047175``
Procedures
----------

Use the CALL statement to perform data manipulation or administrative tasks. Procedures are available in the ``system`` schema of the catalog.
Use the :doc:`/sql/call` statement to perform data manipulation or administrative tasks. Procedures are available in the ``system`` schema of the catalog.

Register Table
^^^^^^^^^^^^^^

Iceberg tables for which table data and metadata already exist in the
file system can be registered with the catalog. Use the ``register_table``
procedure on the catalog's ``system`` schema and supply the target schema,
desired table name, and the location of the table metadata::
procedure on the catalog's ``system`` schema to register a table which
already exists but does not known by the catalog.

The following arguments are available:

===================== ========== =============== =======================================================================
Argument Name required type Description
===================== ========== =============== =======================================================================
``schema`` ✔️ string Schema of the table to register

``table_name`` ✔️ string Name of the table to register

``metadata_location`` ✔️ string The location of the table metadata which is to be registered

``metadata_file`` string An optionally specified metadata file which is to be registered
===================== ========== =============== =======================================================================

Examples:

* Register a table through supplying the target schema, desired table name, and the location of the table metadata::

CALL iceberg.system.register_table('schema_name', 'table_name', 'hdfs://localhost:9000/path/to/iceberg/table/metadata/dir')

CALL iceberg.system.register_table(table_name => 'table_name', schema => 'schema_name', metadata_location => 'hdfs://localhost:9000/path/to/iceberg/table/metadata/dir')

.. note::

If multiple metadata files of the same version exist at the specified
location, the most recently modified one is used.

A metadata file can optionally be included as an argument to ``register_table``
where a specific metadata file contains the targeted table state::
* Register a table through additionally supplying a specific metadata file::

CALL iceberg.system.register_table('schema_name', 'table_name', 'hdfs://localhost:9000/path/to/iceberg/table/metadata/dir', '00000-35a08aed-f4b0-4010-95d2-9d73ef4be01c.metadata.json')

CALL iceberg.system.register_table(table_name => 'table_name', schema => 'schema_name', metadata_location => 'hdfs://localhost:9000/path/to/iceberg/table/metadata/dir', metadata_file => '00000-35a08aed-f4b0-4010-95d2-9d73ef4be01c.metadata.json')

.. note::

The Iceberg REST catalog may not support table register depending on the
Expand All @@ -775,10 +796,24 @@ Unregister Table
^^^^^^^^^^^^^^^^

Iceberg tables can be unregistered from the catalog using the ``unregister_table``
procedure on the catalog's ``system`` schema::
procedure on the catalog's ``system`` schema.

The following arguments are available:

===================== ========== =============== ===================================
Argument Name required type Description
===================== ========== =============== ===================================
``schema`` ✔️ string Schema of the table to unregister

``table_name`` ✔️ string Name of the table to unregister
===================== ========== =============== ===================================

Examples::

CALL iceberg.system.unregister_table('schema_name', 'table_name')

CALL iceberg.system.unregister_table(table_name => 'table_name', schema => 'schema_name')

.. note::

Table data and metadata remain in the filesystem after a call to
Expand Down
16 changes: 16 additions & 0 deletions presto-docs/src/main/sphinx/sql/call.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,22 @@ Procedures can be provided by connectors to perform data manipulation or
administrative tasks. For example, the :doc:`/connector/system` defines a
procedure for killing a running query.

CALL statement supports passing arguments by name or by position. Mixing position and named arguments is not supported.

* Named arguments

Procedure arguments all have a name. When passing arguments by name, arguments can be in any order
and any optional argument can be omitted::

CALL catalog_name.system.procedure_name(arg_name_2 => arg_2, arg_name_1 => arg_1);

* Positional arguments

When passing arguments by position, only the ending arguments may be omitted if they are optional.
Intermediate parameters cannot be omitted::

CALL catalog_name.system.procedure_name(arg_1, arg_2, ... arg_n);

Some connectors, such as the :doc:`/connector/postgresql`, are for systems
that have their own stored procedures. These stored procedures are separate
from the connector-defined procedures discussed here and thus are not
Expand Down
6 changes: 3 additions & 3 deletions ...o-iceberg/src/main/java/com/facebook/presto/iceberg/procedure/RegisterTableProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,9 +84,9 @@ public Procedure get()
"register_table",
ImmutableList.of(
new Procedure.Argument("schema", VARCHAR),
new Procedure.Argument("table", VARCHAR),
new Procedure.Argument("metadataLocation", VARCHAR),
new Procedure.Argument("metadataFile", VARCHAR, false, null)),
new Procedure.Argument("table_name", VARCHAR),
new Procedure.Argument("metadata_location", VARCHAR),
new Procedure.Argument("metadata_file", VARCHAR, false, null)),
REGISTER_TABLE.bindTo(this));
}

Expand Down
2 changes: 1 addition & 1 deletion ...iceberg/src/main/java/com/facebook/presto/iceberg/procedure/UnregisterTableProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public Procedure get()
"unregister_table",
ImmutableList.of(
new Procedure.Argument("schema", VARCHAR),
new Procedure.Argument("table", VARCHAR)),
new Procedure.Argument("table_name", VARCHAR)),
UNREGISTER_TABLE.bindTo(this));
}

Expand Down
2 changes: 1 addition & 1 deletion ...to-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@
import static com.facebook.presto.iceberg.IcebergQueryRunner.getIcebergDataDirectoryPath;
import static com.facebook.presto.iceberg.IcebergUtil.MIN_FORMAT_VERSION_FOR_DELETE;
import static com.facebook.presto.iceberg.procedure.RegisterTableProcedure.METADATA_FOLDER_NAME;
import static com.facebook.presto.iceberg.procedure.TestIcebergRegisterProcedure.getMetadataFileLocation;
import static com.facebook.presto.iceberg.procedure.TestIcebergRegisterAndUnregisterProcedure.getMetadataFileLocation;
import static com.facebook.presto.testing.MaterializedResult.resultBuilder;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.collect.Iterables.getOnlyElement;
Expand Down
46 changes: 42 additions & 4 deletions ...ocedure/TestIcebergRegisterProcedure.java → ...cebergRegisterAndUnregisterProcedure.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertFalse;

public class TestIcebergRegisterProcedure
public class TestIcebergRegisterAndUnregisterProcedure
extends AbstractTestQueryFramework
{
private Session session;
Expand Down Expand Up @@ -126,6 +126,33 @@ public void testRegisterWithMetadataLocation()
dropTable(tableName);
}

@Test
public void testRegisterWithNamedArguments()
{
String tableName = "metadata_loc_named_arguments";
try {
assertUpdate("CREATE TABLE " + tableName + " (id integer, value integer)");
assertUpdate("INSERT INTO " + tableName + " VALUES(1, 1)", 1);

String metadataLocation = getMetadataLocation(TEST_SCHEMA, tableName);
List<String> metadataLocations = Arrays.asList(
metadataLocation, // without trailing slash
format("%s/", metadataLocation), // with training slash
format("%s/metadata", metadataLocation), // with metadata folder specified
format("%s/metadata/", metadataLocation));

for (String validLocation : metadataLocations) {
dropTableFromMetastore(TEST_SCHEMA, tableName);

assertUpdate("CALL system.register_table(metadata_location => '" + validLocation + "', table_name => '" + tableName + "', schema => '" + TEST_SCHEMA + "')");
assertQuery("SELECT * FROM " + tableName, "VALUES (1, 1)");
}
}
finally {
dropTable(tableName);
}
}

@Test
public void testRegisterWithMetadataLocationShowCreate()
{
Expand Down Expand Up @@ -256,10 +283,10 @@ public void testRegisterWithInvalidParameters()
@Language("RegExp") String errorMessage = "line 1:1: Required procedure argument 'schema' is missing";
assertQueryFails("CALL system.register_table()", errorMessage);

errorMessage = "line 1:1: Required procedure argument 'table' is missing";
errorMessage = "line 1:1: Required procedure argument 'table_name' is missing";
assertQueryFails("CALL system.register_table('" + TEST_SCHEMA + "')", errorMessage);

errorMessage = "line 1:1: Required procedure argument 'metadataLocation' is missing";
errorMessage = "line 1:1: Required procedure argument 'metadata_location' is missing";
assertQueryFails("CALL system.register_table('" + TEST_SCHEMA + "', '" + TEST_TABLE_NAME + "')", errorMessage);

errorMessage = "schemaName is empty";
Expand Down Expand Up @@ -373,6 +400,17 @@ public void testUnregisterTable()
assertFalse(getQueryRunner().tableExists(getSession(), tableName));
}

@Test
public void testUnregisterTableWithNamedArguments()
{
String tableName = "unregister";
assertUpdate("CREATE TABLE " + tableName + " (id integer, value integer)");

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

Copy link
Copy Markdown
Member

@agrawalreetika agrawalreetika Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

@hantangwangd hantangwangd Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member

@agrawalreetika agrawalreetika Sep 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me 👍🏻

@Test
public void testRegisterAndUnregisterTable()
{
Expand Down Expand Up @@ -426,7 +464,7 @@ public void testUnregisterWithInvalidParameters()
@Language("RegExp") String errorMessage = "line 1:1: Required procedure argument 'schema' is missing";
assertQueryFails("CALL system.register_table()", errorMessage);

errorMessage = "line 1:1: Required procedure argument 'table' is missing";
errorMessage = "line 1:1: Required procedure argument 'table_name' is missing";
assertQueryFails("CALL system.unregister_table('" + TEST_SCHEMA + "')", errorMessage);

errorMessage = "schemaName is empty";
Expand Down