Add Looker SDK and streaming support #2

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.

core/src/main/java/org/apache/calcite/avatica/remote/looker/LookerRemoteMeta.java

core/src/main/java/org/apache/calcite/avatica/MetaImpl.java

core/src/main/java/org/apache/calcite/avatica/remote/Driver.java

core/src/main/java/org/apache/calcite/avatica/remote/looker/LookerRemoteMeta.java

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.

core/src/main/java/org/apache/calcite/avatica/MetaImpl.java

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

build.gradle.kts

@@ -176,6 +176,8 @@ allprojects {
    plugins.withId("java-library") {
        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.

core/src/main/java/org/apache/calcite/avatica/remote/looker/Driver.java

core/src/main/java/org/apache/calcite/avatica/remote/looker/utils/package-info.java

core/src/main/java/org/apache/calcite/avatica/remote/looker/LookerRemoteMeta.java

core/src/main/java/org/apache/calcite/avatica/remote/looker/utils/LookerSdkFactory.java

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

core/src/test/java/org/apache/calcite/avatica/remote/looker/DriverTest.java

+
+  @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.

core/src/main/java/org/apache/calcite/avatica/remote/looker/LookerRemoteMeta.java

* 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

tjbanghart requested review from mkou, wnob and olivrlee 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
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Looker SDK and streaming support #2

Add Looker SDK and streaming support #2

tjbanghart commented Jul 27, 2023 •

edited

Loading

wnob left a comment

wnob Sep 6, 2023

tjbanghart Sep 7, 2023

wnob Sep 6, 2023

tjbanghart commented Sep 7, 2023

Add Looker SDK and streaming support #2

Add Looker SDK and streaming support #2

Conversation

tjbanghart commented Jul 27, 2023 • edited Loading

wnob left a comment

Choose a reason for hiding this comment

wnob Sep 6, 2023

Choose a reason for hiding this comment

tjbanghart Sep 7, 2023

Choose a reason for hiding this comment

wnob Sep 6, 2023

Choose a reason for hiding this comment

tjbanghart commented Sep 7, 2023

tjbanghart commented Jul 27, 2023 •

edited

Loading