Google Sheet Connector

[chliang71]

Trino has introduced google sheet connector in this PR.

Note that currently Trino google sheet connector only supports read queries, and doesn't support inserts.

This open WIP PR in Trino attempts to add insert functionality. I plan to follow up in next PR.

Related ticket #16516

== RELEASE NOTES ==

General Changes
* Introduce Google sheet connector, supporting read queries

[v-jizhang]

@chliang71 Could you add Co-authors? Thanks

[chliang71]

@chliang71 Could you add Co-authors? Thanks

Thanks for the catch, added co-author and the Trino PR to the commit message

[rohanpednekar]

Hi @xumingming, hope you are doing good. Do you think you can review this PR? Thanks!

[chliang71]

Friendly ping @xumingming @tdcmeehan mind reviewing this PR? Thanks!

[beinan]

Hey @chliang71, it's really a great feature, thanks a lot for your contribution! generally it looks good to me. But the credential file path is hard coded which break the tests. could you fix it?

[beinan]

new line at the end

[beinan]

Hello @chliang71, I think we need a new-line at the end of this pom file just like other connectors.

[beinan]

where is this sheet id coming from? is it a sheet to public?

[chliang71]

Yes this is a public sheet that was created to test this from Trino. I'm reusing the sheet. In production though, users should create their own metadata sheet with right permissioms.

[beinan]

is there an account for this credential? who is the owner of this credential?

[chliang71]

This was the credential for that public testing sheet, it is an encrypted credential. Not sure about the owner, but most likely @punit.kj from the Trinodb PR. This is just for testing.

[beinan]

I see, can we create a new one or reuse the credential of the big-query connector (if there is one)? Thanks!

[beinan]

Also could you update the commit message following the pattern below? thanks!

Fix OOM caused by foo in bar

Foo was pack ratting ByteBuffers causing OOM.

Cherry-pick of https://github.com/foo/bar/pull/123 (https://github.com/foo/bar/pull/123)

Co-authored-by: Foo Bar <foo@bar.com>

[beinan]

@ChunxuTang could you also take a look? I think this connector is also great for twitter's use case. Thanks!

[ChunxuTang]

@beinan Sure, I'm glad to help with a review! This feature looks nice to Twitter.

@chliang71 Thanks so much for your nice work! I left some comments, mainly concentrating on coding styles. Moreover, as this is a new connector, could you create a document and add it to the presto-docs folder?
Thanks! Looking forward to deploying this feature in Twitter!

[beinan]

@chliang71 could you rebase master? Thank you! looks like there is a git conflict

[beinan]

I see, can we create a new one or reuse the credential of the big-query connector (if there is one)? Thanks!

[ChunxuTang]

@chliang71
Reviewed the PR again for the docs. The documentation looks great to me!

[chliang71]

Thanks for the reviews!! @beinan @ChunxuTang . I have rebased to the master, and have updated the commit message. Please let me know if any other comments.

[beinan]

my concern is still the credential stored in presto-google-sheets/src/test/resources/gsheets-creds-base64-encoded.props. I think the owner of this credential is prestosqltest@gmail.com. Also there is a rate limit on this credential, not sure if it will cause any issue if we're this one for the both two presto projects. can we create a new credential under prestodbtest@gmail.com? I can share you the password of this google account offline if you need it.

[chliang71]

my concern is still the credential stored in presto-google-sheets/src/test/resources/gsheets-creds-base64-encoded.props. I think the owner of this credential is prestosqltest@gmail.com. Also there is a rate limit on this credential, not sure if it will cause any issue if we're this one for the both two presto projects. can we create a new credential under prestodbtest@gmail.com? I can share you the password of this google account offline if you need it.

If there is another more fitting credential to use then definitely we can go for it. Thanks.

[chliang71]

@beinan I have updated to use the prestodbtest account, and have recreated the metadata sheet for presto db. Please take a look when you get a chance. Thanks!!

[beinan]

Looks good to me except we should add a new-line at the end of the pom file you newly added.

Also, could you refine the commit message a little bit as https://chris.beams.io/posts/git-commit/ suggested? thanks!

[beinan]

Hello @chliang71, I think we need a new-line at the end of this pom file just like other connectors.

[chliang71]

Thanks for the review @beinan !! I have updated the PR (commit message and pom). But for some reason, although my local build worked fine, the maven check is failing... Please let me know if other changes needed. Thanks!

[beinan]

lgtm, I will rerun the ci

[beinan]

Thanks for the review @beinan !! I have updated the PR (commit message and pom). But for some reason, although my local build worked fine, the maven check is failing... Please let me know if other changes needed. Thanks!

I see, the current version is '0.267-SNAPSHOT' but the pom you just added is still on 0.266-SNAPSHOT

Non-resolvable parent POM for com.facebook.presto:presto-google-sheets:0.266-SNAPSHOT: Could not find artifact com.facebook.presto:presto-root:pom:0.266-SNAPSHOT and 'parent.relativePath' points at wrong local POM @ line 5, column 13 -> [Help 2]

@chliang71 could you please rebase and update the version? Thanks!

[chliang71]

Thanks for checking @beinan , good catch! I updated the pom to 267, now the majority of the checks are passing. But still two test failures

TestDistributedQueuesDb.testDistributedQueue_burstTraffic
TestBrutalShutdown.testQueryRetryOnShutdown

But I don't see how these two tests can be related to my change... also, when I ran these two tests locally, they both passed.

[beinan]

Thanks for checking @beinan , good catch! I updated the pom to 267, now the majority of the checks are passing. But still two test failures

TestDistributedQueuesDb.testDistributedQueue_burstTraffic
TestBrutalShutdown.testQueryRetryOnShutdown

But I don't see how these two tests can be related to my change... also, when I ran these two tests locally, they both passed.

rerun the ci, let's wait and see

[varungajjala]

@beinan We are seeing some unit test failures.

"Expected exception message 'Failed reading data from sheet: 1fu6qjHrOARa7Rqezfgd9C--XPCUOkN_vhvwpxZ4INGE#Tables' to match 'Sheet expression not found for table foo_bar_table' for query: select * from gsheets.default.foo_bar_table" is a sample error message. Can you please take a look and fix them. Builds are broken due to this.

[beinan]

@varungajjala Sure, is this one just flaky or failed all the time?

@chliang71 could you also help? thanks!

[beinan]

@varungajjala I just posted a pr to disable the test: #17052

Looks like these tests are visiting the google service via internet, which I guess would make the test very flaky in poor network environment. I think we could not re-enable these test until we're able to mock the google sheet service. @chliang71 what do you think?