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

DRILL-8235: Add Storage Plugin for Google Sheets #2585

Merged
merged 21 commits into from
Jul 26, 2022

Conversation

cgivre
Copy link
Contributor

@cgivre cgivre commented Jul 5, 2022

DRILL-8235: Add Storage Plugin for Google Sheets

Description

Google Sheets is a very commonly used data source among business users. Presto and other query engines do include integrations with Google Sheets and so it would be useful for Drill to add this functionality.

The proposed plugin supports both reading and writing to Google Sheets.

Here's a quick demo:
https://www.youtube.com/watch?v=pS2FKxrluBA

This PR makes a few addition minor modifications that are not directly related to Google Sheets.

  • Modification to the OAuth token storage to allow plugins besides HTTP to use Drill's token storage mechanism
  • Modification to the OAuth token storage to store the token expiration time.
  • Reapplication of .gitignore and removing some files which should have been ignored. Note this caused a unit test to break, so I had to rename a file to fix this.
  • This PR also integrates DRILL-8270

Documentation

Docs in README.

  • Finalize Unit Tests
  • Fix writer
  • Remove logback file.
  • Code cleanup
  • Fix LGTM bugs

Testing

Manually tested and added unit tests.

@cgivre cgivre added enhancement PRs that add a new functionality to Drill new-storage New Storage Plugin doc-impacting PRs that affect the documentation labels Jul 5, 2022
@cgivre cgivre self-assigned this Jul 5, 2022
@cgivre cgivre requested a review from jnturton July 5, 2022 14:42
@lgtm-com
Copy link

lgtm-com bot commented Jul 5, 2022

This pull request introduces 12 alerts when merging 2817ce2 into 1189e25 - view on LGTM.com

new alerts:

  • 6 for Boxed variable is never null
  • 2 for Spurious Javadoc @param tags
  • 2 for Dereferenced variable may be null
  • 1 for Container contents are never initialized
  • 1 for Implicit narrowing conversion in compound assignment

@jnturton
Copy link
Contributor

jnturton commented Jul 6, 2022

Nice demo vid! Before I dive in, I see that a review has been requested but the PR is also marked draft. Can I switch it over to reviewable? Btw, I like the sheet specification syntax here and it's something I wondered about when doing a bug fix on the Excel format plugin recently. But I guess Excel being a format plugin, this syntax is not available and we have to stick with the format config opt that we have?

@cgivre
Copy link
Contributor Author

cgivre commented Jul 6, 2022

Thanks @jnturton !! You certainly may start reviewing if you like. I left it as draft for the moment as there is still some minor cleanup and debugging that I have to do. I don't anticipate any major changes, but I'm adding more unit tests and fixing all the LGTM / CodeQL issues and stuff like that. If you want to start... please feel free.

@lgtm-com
Copy link

lgtm-com bot commented Jul 7, 2022

This pull request introduces 4 alerts when merging 4f35184 into 1189e25 - view on LGTM.com

new alerts:

  • 2 for Dereferenced variable may be null
  • 1 for Container contents are never initialized
  • 1 for Implicit narrowing conversion in compound assignment

@cgivre cgivre force-pushed the storage-googlesheets branch 2 times, most recently from 7956cec to 9c5389f Compare July 7, 2022 23:24
@lgtm-com
Copy link

lgtm-com bot commented Jul 8, 2022

This pull request introduces 4 alerts when merging 9c5389f into a27eb66 - view on LGTM.com

new alerts:

  • 2 for Dereferenced variable may be null
  • 1 for Container contents are never initialized
  • 1 for Implicit narrowing conversion in compound assignment

@cgivre cgivre force-pushed the storage-googlesheets branch 2 times, most recently from aaad02a to e057d01 Compare July 10, 2022 16:39
@lgtm-com
Copy link

lgtm-com bot commented Jul 10, 2022

This pull request introduces 3 alerts when merging 0862185 into af493aa - view on LGTM.com

new alerts:

  • 1 for Container contents are never initialized
  • 1 for Dereferenced variable may be null
  • 1 for Implicit narrowing conversion in compound assignment

@cgivre cgivre marked this pull request as ready for review July 11, 2022 14:22
@cgivre
Copy link
Contributor Author

cgivre commented Jul 11, 2022

@jnturton This is now ready for review.

@cgivre
Copy link
Contributor Author

cgivre commented Jul 24, 2022

@jnturton Thank you very much for your review of this beast. I believe I have addressed all your comments, as well as fixed the final bugs with the writer.

Copy link
Contributor

@jnturton jnturton 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 all the fixes. Just two minor items remaining from me.

@jnturton jnturton self-requested a review July 25, 2022 17:13
@cgivre cgivre merged commit 9094f14 into apache:master Jul 26, 2022
@cgivre cgivre deleted the storage-googlesheets branch July 26, 2022 00:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-impacting PRs that affect the documentation enhancement PRs that add a new functionality to Drill new-storage New Storage Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants