Support `CREATE TABLE AS SELECT` and `INSERT` in BigQuery by ebyhr · Pull Request #13094 · trinodb/trino

ebyhr · 2022-07-06T01:21:50Z

Description

Support CREATE TABLE AS SELECT and INSERT in BigQuery
Fixes #6868
Fixes #6869

Documentation

(x) Sufficient documentation is included in this PR.

Release notes

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

# BigQuery
* Support `CREATE TABLE AS SELECT` and `INSERT` statements. ({issue}`6868`, {issue}`6869`)

wendigo · 2022-07-07T19:47:56Z

I'm concerned about tabledata.insertAll performance.

As stated in the docs: Streams data into BigQuery one record at a time without needing to run a load job.

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryInsertTableHandle.java

wendigo · 2022-08-17T08:07:16Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

+    public ConnectorOutputTableHandle beginCreateTable(ConnectorSession session, ConnectorTableMetadata tableMetadata, Optional<ConnectorTableLayout> layout, RetryMode retryMode)
+    {
+        if (tableMetadata.getComment().isPresent()) {
+            throw new TrinoException(NOT_SUPPORTED, "This connector does not support creating tables with table comment");


is it bigquery limitation or we just don't support it yet?

It's just connector's limitation.

wendigo · 2022-08-17T08:09:36Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

+        if (isWildcardTable(TableDefinition.Type.valueOf(table.getType()), table.getRemoteTableName().getTableName())) {
+            throw new TrinoException(BIGQUERY_UNSUPPORTED_OPERATION, "This connector does not support inserting into wildcard tables");
+        }
+        List<String> columnNames = columns.stream().map(column -> ((BigQueryColumnHandle) column).getName()).collect(toImmutableList());


JDK 17 has .toList() on a Stream<> which according to Javadoc returns an unmodifiable list. Should we use it @martint ?

For reference: https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/stream/Stream.html#toList()

unmodifiable list and immutablelist semantics are different. null hostility being the big one and unmodifiable isn't really unmodifiable if someone else holds a reference.

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryOutputTableHandle.java

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryPageSink.java

wendigo · 2022-08-17T08:15:07Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryType.java

            return StandardSQLTypeName.TIMESTAMP;
        }
-        if (type instanceof CharType || type instanceof VarcharType) {
+        if (type instanceof VarcharType) {


why this change?

It was required to fix incorrect predicates between char vs varchar if I remember correctly.

What is CharType mapped to now? I don't see explicit mapping so does it mean it's unsupported type now?

It's mapped to BigQuery STRING type now.

I'm probably being stupid but I can't find code which handles that (mapping Trino char to BigQuery STRING). Can you point me to it?

I'm probably confused because of removal of char from createTableSupportedTypes in TestBigQueryConnectorTests.

Sorry, I misunderstood "now" as "before this change". I wanted to say:

CHAR was mapped to STRING before this PR

CHAR is unsupported after this PR

Sorry for causing confusion. 😄 Do we want to mention this in release notes? The only possible impact I see is some CTAS statements from other catalogs might fail but easy to preserve old "incorrect" behaviour by casting CHAR columns to VARCHAR instead.

wendigo

Overall design looks good. We need to improve on type coverage and error handling.

wendigo

lgtm :)

ebyhr · 2022-08-18T07:36:35Z

@hashhar Could you please review this PR when you have time?

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

hashhar · 2022-08-18T16:41:00Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryOutputTableHandle.java

Should this be SchemaTableName or the remote mapped names?

I'm slightly confused now. I see that you didn't have to add any additional code to convert name to remote names. Is it because the remote name logic is contained within BigQueryClient?

If that is the case then your older naming was more correct since the handle will include "Trino" names instead of "remote" names and the BigQueryClient will handle the conversions.

It should be "remote", but a logic to lookup a remote schema name was missing. Could you take a look at https://github.com/trinodb/trino/compare/28038ec2b874963631ae476ca1b9cdcef36e0462..cac3a485decbaebdada4a356c0b50bab68ca34b7?

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryTypeUtils.java

hashhar

I still need to look at type mapping tests.

Also we should add tests for inserts into TestBigQueryCaseInsensitiveMapping (and also for JDBC connectors but that's separate PR).

ebyhr · 2022-08-18T23:07:33Z

@hashhar Addressed comments.

hashhar

LGTM % some questions.

hashhar · 2022-08-19T06:47:25Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

Thank you for catching this. Was this caught by some existing test?

hashhar · 2022-08-19T08:35:36Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryType.java

            return StandardSQLTypeName.TIMESTAMP;
        }
-        if (type instanceof CharType || type instanceof VarcharType) {
+        if (type instanceof VarcharType) {


I'm probably being stupid but I can't find code which handles that (mapping Trino char to BigQuery STRING). Can you point me to it?

I'm probably confused because of removal of char from createTableSupportedTypes in TestBigQueryConnectorTests.

hashhar

Looks good. Thanks.

I'll revisit type mapping and approve.

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

hashhar · 2022-08-19T10:53:08Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

Probably we should also call this in createSchema to avoid creating schemas in BigQuery which differ only in case. Separate and pre-existing issue however.

Turns out this is not needed/the wrong thing to do. When creating schemas we already list them and the listing will include the lowercase name so Trino will see a schema already exists - I've added a test to verify this similar to what you added for DROP SCHEMA in #13812

They are different types, so we shouldn't treat as same type.

* Rename the test method to testInt64 from testInteger * Fix data provider and reorder the entries

ebyhr · 2022-08-19T22:41:47Z

Rebased on upstream to resolve conflicts.

hashhar

LGTM - a question for my understanding.

hashhar · 2022-08-22T05:21:27Z

testing/trino-testing/src/main/java/io/trino/testing/BaseConnectorTest.java

                        "SELECT 1234567890, 1.23",
                "SELECT count(*) + 1 FROM customer");

+        // TODO: BigQuery throws table not found at BigQueryClient.insert if we reuse the same table name


Can you explain a bit more to me? Does this mean that when the DROP TABLE above runs and then we try to CREATE TABLE with same name again here it fails?

Does this mean that when the DROP TABLE above runs and then we try to CREATE TABLE with same name again here it fails?

Yes. Also, the issue doesn't happen in case of empty tables. It seems there's cache or something for BigQuery.insertAll. I couldn't find relevant options in the library at glance. I will look into the detail later.

hashhar

LGTM. Thanks for working on this.

hashhar · 2022-08-23T15:35:32Z

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java

+    @Override
+    public ConnectorOutputTableHandle beginCreateTable(ConnectorSession session, ConnectorTableMetadata tableMetadata, Optional<ConnectorTableLayout> layout, RetryMode retryMode)
+    {
+        return createTable(session, tableMetadata);


Does this also need a defensive check to verify query retries are not enabled?

I think so. Thanks for catching that. Do you want to send the PR?

colebow · 2022-08-24T10:00:14Z

docs/src/main/sphinx/connector/bigquery.rst

 -----------

 The connector provides read and write access to data and metadata in the
-BigQuery database, though write access is limited. In addition to the


cla-bot bot added the cla-signed label Jul 6, 2022

github-actions bot added the docs label Jul 6, 2022

ebyhr marked this pull request as ready for review July 6, 2022 02:56

ebyhr requested review from hashhar and wendigo July 6, 2022 02:57

ebyhr marked this pull request as draft August 3, 2022 05:36

wendigo reviewed Aug 17, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryInsertTableHandle.java Outdated Show resolved Hide resolved

wendigo reviewed Aug 17, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryOutputTableHandle.java Outdated Show resolved Hide resolved

wendigo reviewed Aug 17, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryPageSink.java Outdated Show resolved Hide resolved

wendigo reviewed Aug 17, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryPageSink.java Outdated Show resolved Hide resolved

wendigo reviewed Aug 17, 2022

View reviewed changes

ebyhr marked this pull request as ready for review August 17, 2022 09:09

ebyhr force-pushed the ebi/bigquery-write branch from bf0ea16 to 2ba1e4d Compare August 17, 2022 09:18

wendigo approved these changes Aug 17, 2022

View reviewed changes

ebyhr force-pushed the ebi/bigquery-write branch 2 times, most recently from 23fff3c to d01df23 Compare August 18, 2022 01:57

hashhar reviewed Aug 18, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryClient.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 18, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 18, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryMetadata.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 18, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryTypeUtils.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 18, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryTypeUtils.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 18, 2022

View reviewed changes

plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryTypeUtils.java Outdated Show resolved Hide resolved

hashhar reviewed Aug 18, 2022

View reviewed changes

hashhar approved these changes Aug 19, 2022

View reviewed changes

ebyhr force-pushed the ebi/bigquery-write branch from 28038ec to cac3a48 Compare August 19, 2022 09:08

hashhar reviewed Aug 19, 2022

View reviewed changes

ebyhr added 3 commits August 20, 2022 07:35

Unmap Trino CHAR to BigQuery STRING type

fb5e4d5

They are different types, so we shouldn't treat as same type.

Minor improvement to TestBigQueryTypeMapping integer

cdf273a

* Rename the test method to testInt64 from testInteger * Fix data provider and reorder the entries

Extract a method to get BigQuery remote schema name

426ecb9

ebyhr force-pushed the ebi/bigquery-write branch from cac3a48 to 8d17f02 Compare August 19, 2022 22:41

hashhar approved these changes Aug 22, 2022

View reviewed changes

Support CREATE TABLE AS SELECT and INSERT in BigQuery

6633525

ebyhr force-pushed the ebi/bigquery-write branch from 8d17f02 to 6633525 Compare August 23, 2022 00:38

hashhar approved these changes Aug 23, 2022

View reviewed changes

ebyhr merged commit 80d6b6c into master Aug 23, 2022

ebyhr deleted the ebi/bigquery-write branch August 23, 2022 06:29

ebyhr mentioned this pull request Aug 23, 2022

Release notes for 394 #13723

Closed

github-actions bot added this to the 394 milestone Aug 23, 2022

hashhar reviewed Aug 23, 2022

View reviewed changes

hashhar mentioned this pull request Aug 24, 2022

Some BigQuery cleanups #13812

Merged

colebow mentioned this pull request Aug 24, 2022

Add Trino 394 release notes #13813

Merged

colebow reviewed Aug 24, 2022

View reviewed changes

Conversation

ebyhr commented Jul 6, 2022

Description

Documentation

Release notes

Uh oh!

wendigo commented Jul 7, 2022

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!

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!

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!

wendigo left a comment

Choose a reason for hiding this comment

Uh oh!

wendigo left a comment

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Aug 18, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hashhar Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

Uh oh!

ebyhr commented Aug 18, 2022

Uh oh!

hashhar 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!

hashhar left a comment

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!

ebyhr commented Aug 19, 2022

Uh oh!

hashhar left a comment

Choose a reason for hiding this comment

hashhar Aug 19, 2022 •

edited

Loading