feat(java/driver/flight-sql): implement getObjects #1517

jduo · 2024-02-05T21:17:27Z

Implement getObjects and each depth (CATALOGS, DB_SCHEMAS, TABLES, and ALL).
Handle the cases where the catalog is empty or a schema within a catalog has no tables.
Unify readers for queries, getInfo(), and getObjects() so that all code paths can correctly get data when the data is not available on the same location as the original connection, can handle multiple roots from the same stream, and can handle multiple partitions.
Rework getInfo() request to lazily issue an RPC call only if a request code was used that needs information from the Flight server.

Fixes #745

jduo · 2024-02-05T21:18:54Z

This incorporates the work from #605 . However I'm examining the Go Flight SQL ADBC driver to see if I can port some of its implementation here to cover the cases missing (empty catalogs or schemas).

tokoko · 2024-02-06T00:22:44Z

@jduo yeah, either that or you will have to preprocess to get those missing cases and add them in correctly sorted order during normal processing. Good thing is that you won't have to keep much of it in some local object in-memory unlike go implementation but the code might get really ugly.

jduo · 2024-02-08T01:11:48Z

For this iteration I'm going to assume the server returns data in the correct order and doesn't do anything crazy such as split the columns from one table across separate partitions.

jduo · 2024-02-14T19:13:39Z

I'm almost done writing the implementation of this. It turned out to be a fairly complicated ordeal to compress/denormalize the flattened data from Flight SQL to ADBC's nested lists of structs.

A couple of things:

I'm assuming we should not use SqlQuirks to map Arrow types to type names for Flight SQL, since Flight SQL is already Arrow-designed and this should probably have happened in the Flight SQL Producer already.
If the user uses Depth ALL to get column metadata, and supplies a column pattern that filters out all columns, I am assuming the result should be that the table is still listed but the columns list is non-null but empty (as opposed to the table being filtered out entirely or the table being listed but the columns list being null).

…tionTest

* Change GetSqlInfo and GetObjects to use the same reader as FlightInfoReader. This fixes bugs where locations aren't used for these metadata calls and also makes them use resources lazily. * Fix a bug in the FlightInfoReader path where the first stream is assumed to be from the current client rather than using a location in the first endpoint. * Fix handling of empty schemas or empty collections when using GetObjects.

Fix NPEs and not propagating credentials for some calls

jduo · 2024-02-17T14:51:16Z

This PR supports getting catalogs, schemas, tables, and columns. Tests include:

catalogs with multiple schemas.
schemas with multiple tables
empty catalogs
catalogs with empty schemas
filtering by catalog name
filtering by column name
filtering by catalog such that an empty result is returned
filtering by column name such that no columns are returned
skipping filtering by catalog name and column name.

Also call root.allocateNew() in GetInfoReader

lidavidm · 2024-02-15T17:21:43Z

java/driver/flight-sql/pom.xml

+    <!-- Helpers for mapping Arrow types to ANSI SQL types and building test servers -->
+    <dependency>
+      <groupId>org.apache.arrow</groupId>
+      <artifactId>flight-sql-jdbc-core</artifactId>


This is a heavy dependency to have to pull in, unfortunately

lidavidm · 2024-02-18T14:58:45Z

...driver/flight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/BaseFlightReader.java

+  // Checker framework is considering Arrow functions such as FlightStream.next() as potentially
+  // altering the state
+  // and able to change currentStream or schema fields to null.


FWIW, usually I've gotten around this by doing the equivalent of

if (field == null) { throw; } T localField = field; // never null now

lidavidm · 2024-02-18T14:59:45Z

...r/flight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/GetInfoMetadataReader.java

+          // TODO: actual version
+          setStringValue(dstIndex++, "0.0.1".getBytes(StandardCharsets.UTF_8));


Can we file an issue for this? (I see it was there originally)

lidavidm · 2024-02-18T15:01:58Z

...ight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/GetObjectsMetadataReaders.java

+      if (JAVA_REGEX_SPECIALS.indexOf(currentChar) >= 0) {
+        javaPattern.append('\\');
+      }


Don't you only want to do this if it's not a _ or %?

Also, you could just always apply Pattern.quote instead of hardcoding the potential special characters?

lidavidm · 2024-02-18T15:02:48Z

...ight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/GetObjectsMetadataReaders.java

+      // This is taken from the JDBC driver, but seems wrong that all three branches write the same
+      // value.
+      // Float should probably be 2.


Can we file an issue?

lidavidm · 2024-02-18T15:04:16Z

...ight-sql/src/main/java/org/apache/arrow/adbc/driver/flightsql/GetObjectsMetadataReaders.java

+    @SuppressWarnings(
+        "method.invocation") // Checker Framework does not like the ensureInitialized call


It needs to be properly annotated upstream (CC @davisusanibar)

lidavidm · 2024-02-18T15:09:38Z

.../driver/flight-sql/src/test/java/org/apache/arrow/adbc/driver/flightsql/GetObjectsTests.java

+import org.junit.AfterClass;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;


We've been using JUnit5, can we stick to that? Or does the TestRule force us onto JUnit 4? (If so, can we file issues to fix this?)

github-actions bot added this to the ADBC Libraries 0.10.0 milestone Feb 5, 2024

jduo mentioned this pull request Feb 5, 2024

feat(java/driver/flight-sql): implement getObjects #605

Closed

jduo self-assigned this Feb 7, 2024

jduo force-pushed the java-flight-sql-objects branch from bc44112 to dfff096 Compare February 9, 2024 20:53

lidavidm modified the milestones: ADBC Libraries 0.10.0, ADBC Libraries 0.11.0 Feb 14, 2024

jduo force-pushed the java-flight-sql-objects branch from 2e0009a to e527e25 Compare February 15, 2024 14:24

tokoko and others added 11 commits February 17, 2024 06:20

feat(java/driver/fligh-sql): implement getObjects

0bfc9a9

feat(java/driver/flight-sql): add missing license to FlightSqlTransac…

a2d5ed1

…tionTest

feat(java/driver/fligh-sql): precompile columnNamePattern

0eaf46d

Fix some leaks

da522ef

Linter fixes

45ea711

Revert changes relating to TransactionTest

abba3e5

Ensure tests using GetInfo request data from the server

fac525b

Support Depth.TABLES and Depth.ALL

d737c0c

Fix some failures

c20e67f

Fix NPEs and not propagating credentials for some calls

Fix getting columns and add more tests

c90881d

jduo force-pushed the java-flight-sql-objects branch from 50051ba to c90881d Compare February 17, 2024 14:20

Checker framework fixes

47caa0e

jduo marked this pull request as ready for review February 17, 2024 14:47

jduo requested a review from lidavidm as a code owner February 17, 2024 14:47

jduo added 3 commits February 17, 2024 07:14

Linter

f540c72

Also call root.allocateNew() in GetInfoReader

Fix GetInfo test failures

2744720

Linter

8c4d414

jduo added 3 commits February 17, 2024 08:17

Checker framework fixes

711978e

Checkerframework

94e3a1c

Fix checker framework

517aad2

jduo mentioned this pull request Feb 18, 2024

java: bring Flight SQL driver on par with Go Flight SQL driver #745

Closed

lidavidm modified the milestones: ADBC Libraries 0.11.0, ADBC Libraries 0.10.0 Feb 18, 2024

lidavidm approved these changes Feb 18, 2024

View reviewed changes

lidavidm merged commit adfb70a into apache:main Feb 18, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(java/driver/flight-sql): implement getObjects #1517

feat(java/driver/flight-sql): implement getObjects #1517

jduo commented Feb 5, 2024 •

edited

Loading

jduo commented Feb 5, 2024

tokoko commented Feb 6, 2024

jduo commented Feb 8, 2024

jduo commented Feb 14, 2024

jduo commented Feb 17, 2024

lidavidm Feb 15, 2024

lidavidm Feb 18, 2024

lidavidm Feb 18, 2024

lidavidm Feb 18, 2024

lidavidm Feb 18, 2024

lidavidm Feb 18, 2024

lidavidm Feb 18, 2024

		// TODO: actual version
		setStringValue(dstIndex++, "0.0.1".getBytes(StandardCharsets.UTF_8));

		@SuppressWarnings(
		"method.invocation") // Checker Framework does not like the ensureInitialized call

feat(java/driver/flight-sql): implement getObjects #1517

feat(java/driver/flight-sql): implement getObjects #1517

Conversation

jduo commented Feb 5, 2024 • edited Loading

jduo commented Feb 5, 2024

tokoko commented Feb 6, 2024

jduo commented Feb 8, 2024

jduo commented Feb 14, 2024

jduo commented Feb 17, 2024

lidavidm Feb 15, 2024

Choose a reason for hiding this comment

lidavidm Feb 18, 2024

Choose a reason for hiding this comment

lidavidm Feb 18, 2024

Choose a reason for hiding this comment

lidavidm Feb 18, 2024

Choose a reason for hiding this comment

lidavidm Feb 18, 2024

Choose a reason for hiding this comment

lidavidm Feb 18, 2024

Choose a reason for hiding this comment

lidavidm Feb 18, 2024

Choose a reason for hiding this comment

jduo commented Feb 5, 2024 •

edited

Loading