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.

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

Jump to bottom

BigQuery Connector #2532

Closed
wants to merge 26 commits into from
Closed

BigQuery Connector #2532

wants to merge 26 commits into from

Conversation

davidrabinowitz
Copy link
Contributor

Implementing a connector for BigQuery.

Instructions on how to use it are specified in the documentation - https://github.com/davidrabinowitz/prestosql/blob/master/presto-docs/src/main/sphinx/connector/bigquery.rst

@cla-bot cla-bot bot added the cla-signed label Jan 16, 2020
ebyhr
ebyhr reviewed Jan 22, 2020
View reviewed changes
Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Thanks for creating this PR. Only left basic comments as the initial review.

presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryConfig.java Outdated Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryMetadata.java Outdated Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryPageSource.java Outdated Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryPageSource.java Outdated Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryStorageClientFactory.java Outdated Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryColumnHandle.java Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryUtil.java Outdated Show resolved Hide resolved
presto-bigquery/src/test/java/io/prestosql/plugin/bigquery/MockResponsesBatch.java Outdated Show resolved Hide resolved
pom.xml Outdated Show resolved Hide resolved
@davidrabinowitz
Copy link
Contributor Author

davidrabinowitz commented Jan 30, 2020 •
edited
Loading

After making the POM changes I have this warning I can't rid off:

[INFO] Found Resolved Dependency/DependencyManagement mismatches:
[INFO] 	Ignoring Direct Dependencies.
[INFO] com.fasterxml.jackson.core:jackson-core:jar was excluded in DepMgt, but version 2.10.0 has been found in the dependency tree.
[INFO] org.apache.httpcomponents:httpclient:jar was excluded in DepMgt, but version 4.5.10 has been found in the dependency tree.
[INFO] org.apache.httpcomponents:httpcore:jar was excluded in DepMgt, but version 4.4.12 has been found in the dependency tree.
[INFO] org.slf4j:slf4j-api:jar was excluded in DepMgt, but version 1.7.29 has been found in the dependency tree.
[INFO] joda-time:joda-time:jar was excluded in DepMgt, but version 2.10.5 has been found in the dependency tree.
[WARNING] Potential problems found in Dependency Management

The jars exist in the connector's zip. Any idea?

@davidrabinowitz davidrabinowitz requested a review from ebyhr February 4, 2020 23:43
@electrum
Copy link
Member

electrum commented Feb 4, 2020

Don't worry about those dependency management warnings. They're bogus and can be ignored. I have a PR to remove that checker.

electrum
electrum reviewed Feb 4, 2020
View reviewed changes
presto-bigquery/pom.xml Outdated Show resolved Hide resolved
@davidrabinowitz davidrabinowitz requested review from electrum and martint and removed request for kokosing February 5, 2020 18:45
electrum
electrum reviewed Feb 10, 2020
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

First pass, lots of comments, mostly minor

presto-bigquery/pom.xml Outdated Show resolved Hide resolved
presto-bigquery/pom.xml Outdated Show resolved Hide resolved
presto-bigquery/pom.xml Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryColumnHandle.java Outdated Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryColumnHandle.java Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/bigquery.rst Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/bigquery.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/bigquery.rst Outdated Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/bigquery.rst Outdated Show resolved Hide resolved
presto-main/etc/config.properties Outdated Show resolved Hide resolved
@davidrabinowitz davidrabinowitz mentioned this pull request Feb 25, 2020
Closed
@davidrabinowitz
Copy link
Contributor Author

@electrum @findepi All the issues have been fixed

@davidrabinowitz
fixed TestBigQueryConfig
192b5cd
@davidrabinowitz
fixed TestBigQueryConfig - lazy loading default project id
48cddd3
@davidrabinowitz
update version
0cd0e98
@davidrabinowitz
fixing post-merge issues
94246fa
@davidrabinowitz
Applied @electrum comments
8e5fbad
@davidrabinowitz
resolving coding style issues raised by @electrum
22cc015
@davidrabinowitz
resolving coding style issues raised by @electrum
4b4828c
@davidrabinowitz
fixed doc
3a9f790
@davidrabinowitz
resolving coding and doc style issues raised by @electrum
debd2d2
@davidrabinowitz
fixing cehckstyle issues
ce4e215
@davidrabinowitz
More fixes, added TestBigQueryIntegrationSmokeTest
78dba25
@davidrabinowitz
Resolving @electrum comments, implementing empty projection for count(*)
c76755b
@davidrabinowitz
Upgrage to 331-SNAPSHOT, fix style issues
a013b88
@davidrabinowitz
Fixing trinodb#2532 (comment)
9860640
@davidrabinowitz
BigQuery datetime is now represented by Presto timestamp, as asked in t…
9e06909 
…rinodb#2532 (comment)
@davidrabinowitz
Supporting tables and datasets with uppper case characters, correct h…
ea18ab7 
…andling of information_schema. The integration smoke test now works
@davidrabinowitz
fixing code format
4a92bd0
@davidrabinowitz
Fixing documentation
8415489
@davidrabinowitz
Fixing documentation
07d3da5
@davidrabinowitz
table type is now correctly shown in the information_schema
bcdfad9
electrum
electrum approved these changes Feb 29, 2020
View reviewed changes
Copy link
Member

@electrum electrum left a comment

Choose a reason for hiding this comment

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

I'm going to fix up these few small issues and merge it, so we can get this in for the next release.

One issue I found in testing is that you have to set both project-id and parent-project-id, but the error comes from the Google library and just says "Project ID", so it's confusing. Also, I'm not sure why I would need to set the parent unless I want separate billing, but I'm completely new to GCP so maybe it makes sense.

Also, can I get access to, or can you tell me how to create, the TPCH tables needed for TestBigQueryIntegrationSmokeTest?

presto-main/etc/config.properties Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/BigQueryConfig.java Show resolved Hide resolved
presto-docs/src/main/sphinx/connector/bigquery.rst Show resolved Hide resolved
presto-bigquery/src/test/java/io/prestosql/plugin/bigquery/BigQueryQueryRunner.java Show resolved Hide resolved
presto-bigquery/src/main/java/io/prestosql/plugin/bigquery/Conversions.java Show resolved Hide resolved
@electrum
Copy link
Member

Merged, thanks!

@electrum electrum closed this Feb 29, 2020
@findepi findepi added this to the 331 milestone Mar 4, 2020
@electrum electrum mentioned this pull request Mar 11, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@electrum electrum electrum approved these changes

@findepi findepi Awaiting requested review from findepi

@ebyhr ebyhr Awaiting requested review from ebyhr

@martint martint Awaiting requested review from martint

Assignees
No one assigned
Labels
cla-signed
Milestone
331
Development

Successfully merging this pull request may close these issues.
4 participants
@davidrabinowitz @electrum @findepi @ebyhr