Add Looker SDK and streaming support by tjbanghart · Pull Request #2 · looker-open-source/calcite-avatica

tjbanghart · 2023-07-27T21:26:19Z

Adds functionality described in go/scaling-avatica-looker-api.

To enable the the Looker SDK use jdbc:looker (as opposed to jdbc:avatica:remote) as the JDBC URL protocol.
NOTE: This is awaiting changes from b/288031194. Don't expect this to work properly without that work completed.

TODOs:

Unit tests for everything but definitely the streaming parser.
Use sql_interface endpoints.
Ensure OAuth tokens are set as AuthTokens rather than raw header.
Handle metadata from json_bi result format. We currently skip right to rows.
Support prepared statements. i.e. those that Prepare then later Execute. Currently we only override PrepareAndExecute
Kotlin: support streaming sdk-codegen#1341 - not going to happen for awhile. Requires large refactor to the SDK gen.

wnob

Leaving some general readability comments for now. Will think harder about this later. Since this is going to be a permanent "fixup" commit, we should be pretty careful about making sure it remains easy to rebase on master for every bump, so good to think about exactly how to break it into smaller commits for easy maintenance in posterity.

wnob · 2023-09-06T18:30:59Z

        dependencies {
            "implementation"(platform(project(":bom")))
+            // Add the locally bundled LookerSDK fat jar
+            "implementation"(files("../libs/looker-kotlin-sdk-f91f8ad.jar"))


Feels like we should file a cleanup bug to make this integration easier and throw it in the backlog for now.

Yeah definitely - we will be building this jars locally for the foreseeable future. There was talk of getting this on maven and tied to official releases but I don't think that will happen soon.

wnob · 2023-09-06T21:01:51Z

+
+  @Test
+  public void driverThrowsAuthExceptionForBlankProperties() throws SQLException {
+    Properties props = new Properties();


http://go/unit-test-practices#structure

I find the arrange - act - assert structure does a lot for test readability. Just put some blank space in between each section. It's OK if some tests are more like arrange - act - assert - act - assert.

tjbanghart · 2023-09-07T02:16:37Z

Thanks for the review @wnob!

I am still exploring what we would need to have upstream, but my hope is that we can keep all meaningful changes in org.apache.calcite.avatica.remote.looker. I would like this to be an extension of the remote Avatica driver rather than a fixup to existing implementations. There might be some changes here that others would benefit from, but I don't believe much of this should make it into main with the exception of some class + method visibility changes (which do need Jira cases).

That said, response streaming into a series of Frames may be something to lift out of this and would probably benefit most from open-source contributions. Perhaps it makes sense to open a Jira case for this as well... on the other hand the streamed response is specific to Looker's json_bi format.

IMO, Avatica has been open enough that we have not had to modify existing classes extensively. Any areas where this PR has done such modifications highlight potential Jira cases for upstream.

* Initial Looker SDK branch commit * Streaming LookerIterator * True streaming support * Make a specific Driver class and 'jdbc:looker:' protocol support * Add read timeout for stream * SQL Interface API endpoints * Use BlockingQueue of FrameEnvelopes rather than IO pipes * Fix illegal cast for queries with a single column * Stub driver for LookerRemoteMetaTest * Address feedback * Handle null values in stream * Rename driver to LookerDriver * Better error handling from API response * Support userAgent and make LookerResponseParser public for testing

# This is the 1st commit message: trigger ci # This is the commit message #2: Updating to support new Kotlin SDK with IAP suport and updating Kotlin SDK shadow jar packaged in libs # This is the commit message #3: Updating Kotlin Jar for java 8 # This is the commit message #4: Removing redundant code in createSdk # This is the commit message #5: Adding new Jar and IAP functionality Adding test Added additional tests to LookerSdkFactoryTest.java Updating testing Fixing checkstyle Fixing checkstyle Fixing checkstyle Fixing styling based on PR comments and adding props size to hashmap in createSdk adding update to assert message Updating tests for IAP Updating tests for IAP removed unneeded email decode check removed unneeded email decode check

tjbanghart requested review from mkou, olivrlee and wnob July 27, 2023 21:26

barrkel reviewed Jul 31, 2023

View reviewed changes

tjbanghart force-pushed the looker branch from 961a582 to 8d06529 Compare August 9, 2023 20:34

tjbanghart force-pushed the tjbanghart/looker-sdk branch from b407594 to 1e914b6 Compare August 9, 2023 20:51

wnob reviewed Sep 6, 2023

View reviewed changes

tjbanghart force-pushed the tjbanghart/looker-sdk branch from 78906b2 to aa93bd9 Compare September 8, 2023 16:55

tjbanghart requested review from barrkel and wnob September 8, 2023 16:57

barrkel reviewed Sep 12, 2023

View reviewed changes

tjbanghart force-pushed the tjbanghart/looker-sdk branch from dc7ec93 to 24e2532 Compare September 14, 2023 05:15

tjbanghart requested a review from barrkel September 15, 2023 05:10

barrkel approved these changes Sep 18, 2023

View reviewed changes

tjbanghart force-pushed the looker branch from 8b5f05a to e1bdd40 Compare September 20, 2023 05:10

tjbanghart added 14 commits September 19, 2023 22:12

Initial Looker SDK branch commit

1ba037e

Streaming LookerIterator

8fdb46d

True streaming support

8901237

Make a specific Driver class and 'jdbc:looker:' protocol support

67ce455

Add read timeout for stream

b7469b5

SQL Interface API endpoints

fde0bda

Use BlockingQueue of FrameEnvelopes rather than IO pipes

5857ed3

Fix illegal cast for queries with a single column

ccfa082

Stub driver for LookerRemoteMetaTest

fc0244b

Address feedback

86b4c27

Handle null values in stream

5e71344

Rename driver to LookerDriver

fbaadb5

Better error handling from API response

c1a8773

Support userAgent and make LookerResponseParser public for testing

d9826eb

tjbanghart force-pushed the tjbanghart/looker-sdk branch from b53d4d6 to d9826eb Compare September 20, 2023 05:13

tjbanghart changed the title ~~WIP: Add Looker SDK and streaming support~~ Add Looker SDK and streaming support Sep 20, 2023

tjbanghart merged commit dbe2821 into looker Sep 20, 2023

Conversation

tjbanghart commented Jul 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wnob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wnob Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

tjbanghart Sep 7, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wnob Sep 6, 2023

Choose a reason for hiding this comment

Uh oh!

tjbanghart commented Sep 7, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tjbanghart commented Jul 27, 2023 •

edited

Loading