Skip to content
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.

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

Document SQL Support for the Iceberg connector #13558

Merged
merged 2 commits into from
Oct 14, 2022

Conversation

findinpath
Copy link
Contributor

Description

Document SQL support of the Iceberg connector.

Is this change a fix, improvement, new feature, refactoring, or other?

N/A

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

N/A

How would you describe this change to a non-technical end user or system administrator?

Document SQL support of the Iceberg connector.

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
(x) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

(x) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Aug 9, 2022
@findinpath findinpath added the docs label Aug 9, 2022
@findinpath findinpath force-pushed the docs/iceberg-sql-support branch 2 times, most recently from badc22f to 0fc08fe Compare August 9, 2022 09:29
@findinpath findinpath requested a review from claudiusli August 9, 2022 09:32
@ebyhr ebyhr requested a review from mosabua August 9, 2022 10:32
@findinpath findinpath force-pushed the docs/iceberg-sql-support branch from 0fc08fe to b38ae17 Compare August 9, 2022 16:21
Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

I will review again once initial comment is addressed.

docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
* :ref:`sql-schema-table-management`, see also :ref:`iceberg-tables`
* :ref:`sql-materialized-view-management`, see also
:ref:`iceberg-materialized-views`
* :ref:`sql-materialized-view-management`, see also :ref:`iceberg-materialized-views`
Copy link
Member

Choose a reason for hiding this comment

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

This is correct ..

@findinpath findinpath force-pushed the docs/iceberg-sql-support branch 2 times, most recently from b937738 to de385a1 Compare September 6, 2022 10:52
@findinpath findinpath requested a review from mosabua September 6, 2022 10:54
@@ -50,6 +50,9 @@ metastore service (HMS) or AWS Glue. The catalog type is determined by the
``iceberg.catalog.type`` property, it can be set to either ``HIVE_METASTORE``
or ``GLUE``.


.. _iceberg-hive-catalog:

Hive metastore catalog
Copy link
Member

Choose a reason for hiding this comment

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

Change to

Hive metastore service (HMS)

or just

Hive metastore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is about the Iceberg catalog implementation which is backed by HMS. In this case I tend to think that the name is suitable. NO

Copy link
Member

Choose a reason for hiding this comment

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

Sure .. but in the context of Trino its confusing to use catalog for two different concepts .. not sure how to get around this better

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

A couple of changes .. some of the examples seem to just repeat the docs on how a specific SQL statement works but I guess thats fine..

docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
COMMENT
~~~~~~~

The Iceberg connector supports setting comments on the following objects:
Copy link
Member

Choose a reason for hiding this comment

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

could also just do

on tables, views, and columns with the COMMENT statement.

but .. also ALTER x ADD COMMENT and such works too right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ALTER x ADD COMMENT I'm not aware of this syntax. I haven't seen also any occurrence of "ADD COMMENT" in the codebase.

Copy link
Member

Choose a reason for hiding this comment

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

Sorry for not being clear .. I meant this stuff for example

ALTER TABLE [ IF EXISTS ] name ADD COLUMN [ IF NOT EXISTS ] column_name data_type
[ NOT NULL ] [ COMMENT comment ]

But also setting comments when using create

CREATE TABLE [ IF NOT EXISTS ]
table_name (
{ column_name data_type [ NOT NULL ]
[ COMMENT comment ]
[ WITH ( property_name = expression [, ...] ) ]
| LIKE existing_table_name
[ { INCLUDING | EXCLUDING } PROPERTIES ]
}
[, ...]
)
[ COMMENT table_comment ]

In that case .. table and column comments..

Copy link
Member

Choose a reason for hiding this comment

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

We should verify and document that they all work for tables and views.. also what about materialized views?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added corresponding mentions for both CREATE TABLE , ALTER TABLE. Thanks for the suggestion.

Setting comments on MV is not supported.

docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
@findinpath findinpath force-pushed the docs/iceberg-sql-support branch from de385a1 to 8cb457b Compare September 8, 2022 14:42
@findinpath findinpath requested a review from mosabua September 12, 2022 11:14
is stored in a subdirectory under the directory corresponding to the
schema location.

The Iceberg connector supports creating tables using the :doc:`CREATE
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we need all this, but lets leave it since you already have it.

@findinpath findinpath force-pushed the docs/iceberg-sql-support branch from 8cb457b to d1f2520 Compare September 27, 2022 11:47
@findinpath
Copy link
Contributor Author

@mosabua CPTAL at the updated PR?

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

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

Lets ship this. Nice work

WITH (
format = 'PARQUET',
partitioning = ARRAY['c1', 'c2'],
location = 's3://my-bucket/a/path/'
Copy link
Member

Choose a reason for hiding this comment

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

A trailing slash caused a regression in the past versions. Let's avoid using it in the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trino> CREATE SCHEMA iceberg.tiny WITH (location='s3a://tiny');
Query 20221007_094114_00002_ct5a7 failed: java.lang.IllegalArgumentException: Can not create a Path from an empty string
io.trino.spi.TrinoException: java.lang.IllegalArgumentException: Can not create a Path from an empty string
	at io.trino.plugin.hive.metastore.thrift.ThriftHiveMetastore.createDatabase(ThriftHiveMetastore.java:963)
	at io.trino.plugin.hive.metastore.thrift.BridgingHiveMetastore.createDatabase(BridgingHiveMetastore.java:160)
	at io.trino.plugin.hive.metastore.cache.CachingHiveMetastore.createDatabase(CachingHiveMetastore.java:450)
	at io.trino.plugin.iceberg.catalog.hms.TrinoHiveCatalog.createNamespace(TrinoHiveCatalog.java:184)
	at io.trino.plugin.iceberg.IcebergMetadata.createSchema(IcebergMetadata.java:609)
	at io.trino.plugin.base.classloader.ClassLoaderSafeConnectorMetadata.createSchema(ClassLoaderSafeConnectorMetadata.java:350)
	at io.trino.metadata.MetadataManager.createSchema(MetadataManager.java:592)
...

I've tried on Trino & Hive & MinIO this functionality without a trailing / and stumbled on the previously exposed issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

trino> create table iceberg.tiny.test1 (x integer) with (location='s3a://tiny/test1');
CREATE TABLE
trino> create table iceberg.tiny.test2 (x integer) with (location='s3a://tiny/test2/');
CREATE TABLE

On the table however, I see that creating a table is possible with and without trailing slash.
Probably the above mentioned situation is a bug.

* :ref:`sql-view-management`
* :ref:`sql-write-operations`:

* :ref:`sql-schema-table-management`, see also :ref:`iceberg-schema-table-management` and :ref:`iceberg-tables`
Copy link
Member

Choose a reason for hiding this comment

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

Generated page has repeated "Schema and table management". By the way, why do we want both sql-schema-table-management & iceberg-schema-table-management?

Screen Shot 2022-10-06 at 14 35 42

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need both. Actually having the links on the same line is misleading.
The iceberg-schema-table-management has anyway a link towards sql-schema-table-management so taking out the sql-schema-table-management link from this listing probably ok.

docs/src/main/sphinx/connector/iceberg.rst Outdated Show resolved Hide resolved
@findinpath findinpath force-pushed the docs/iceberg-sql-support branch from d1f2520 to dc3a7da Compare October 7, 2022 09:44
@findinpath findinpath requested a review from ebyhr October 13, 2022 09:55
Comment on lines +295 to +297
c1 integer,
c2 date,
c3 double
Copy link
Member

@ebyhr ebyhr Oct 13, 2022

Choose a reason for hiding this comment

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

nit: Type names are lowercase here, but the following DDL for NOT NULL example uses uppercase. Follow-up PR is fine.

@ebyhr ebyhr merged commit 51d2413 into trinodb:master Oct 14, 2022
@ebyhr
Copy link
Member

ebyhr commented Oct 14, 2022

Merged, thanks!

@github-actions github-actions bot added this to the 401 milestone Oct 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants