feat: Add Flight SQL server support #6023

devinrsmith · 2024-09-05T22:25:47Z

This adds Arrow Flight SQL support to the Deephaven server, with the goal of supporting various SQL drivers built on top of Flight SQL (JDBC, ADBC, ODBC). It is limited to query statements (ie, no UPDATEs). The implementation supports ad-hoc query statements and ad-hoc looking prepared query statements, but not parameterized prepared statements. Queries are able to reference tables from the global scope by name; we might be able to expand support to other resolvers in the future, potentially with catalogs and namespaces. The scope of supported queries is guided by our Sql engine io.deephaven.engine.sql.Sql which is based on a Calcite query parser.

It is important to note that while the Flight SQL implementation respects io.deephaven.server.session.TicketResolver.Authorization.transform it is not hooked up to io.deephaven.auth.ServiceAuthWiring nor io.deephaven.server.table.validation.ColumnExpressionValidator. So while Flight SQL does not allow users script execution, it does not limit the table operations a user may perform, nor does it restrict the calling of arbitrary functions from a formula. As such, the security posture of Flight SQL sits somewhere between "full access" and "limited access".

A cookie-based authentication extension has been added to support some Flight SQL clients which don't operate via the normal Flight authentication "Authorization" header (yes, it's a misnomer), and instead expect the server to send "Set-Cookie" to the clients, and for the clients to echo back the cookie(s) via the "Cookie" header. The server will only send the authentication token as a cookie when the client sends the header and value "x-deephaven-auth-cookie-request=true". To support this, the io.grpc.Context has been captured and preserved during the export submission logic.

The full Flight SQL action and command spectrum has been skeleton-ed out with appropriate "unimplemented" messages in anticipation that we will want to expand the scope of the Flight SQL implementation in the future. It also serves as a more explicit guide to readers of the code for what is and is not supported.

Fixes #5339

chipkent

py dir LGTM

server/src/main/java/io/deephaven/server/arrow/FlightServiceGrpcImpl.java

server/src/main/java/io/deephaven/server/session/SessionState.java

server/src/main/java/io/deephaven/server/session/TicketResolver.java

server/src/main/java/io/deephaven/server/session/TicketRouter.java

flightsql/src/main/java/io/deephaven/server/flightsql/TableCreatorScopeTickets.java

flightsql/src/main/java/io/deephaven/server/flightsql/FlightSqlTicketResolver.java

chipkent

py dir LGTM.

See comment in deephaven#4575

niloc132

Warnings in tests in java 11, will be failures in 17+:

> Task :extensions-flight-sql:jdbcTest
1 compiler directives added

WARNING: An illegal reflective access operation has occurred
WARNING: Illegal reflective access by cfjd.org.apache.calcite.avatica.com.google.protobuf.UnsafeUtil (file:/home/colin/.gradle/caches/modules-2/files-2.1/org.apache.arrow/flight-sql-jdbc-driver/13.0.0/179d3073933291f6b7b1584b56d8b97f0ae1f740/flight-sql-jdbc-driver-13.0.0.jar) to field java.lang.String.value
WARNING: Please consider reporting this to the maintainers of cfjd.org.apache.calcite.avatica.com.google.protobuf.UnsafeUtil
WARNING: Use --illegal-access=warn to enable warnings of further illegal reflective access operations
WARNING: All illegal access operations will be denied in a future release

engine/updategraph/src/main/java/io/deephaven/engine/liveness/Liveness.java

extensions/barrage/src/main/java/io/deephaven/extensions/barrage/util/BarrageUtil.java

niloc132 · 2024-11-11T18:18:08Z

extensions/flight-sql/build.gradle

+        compileClasspath += sourceSets.main.output
+        compileClasspath += sourceSets.test.output
+
+        runtimeClasspath += sourceSets.main.output
+        runtimeClasspath += sourceSets.test.output


None of this seems to be necessary - both in that jdbcTest doesnt depend on test at all (should be using the java-test-fixtures plugin to share like that), and that the remaining lines can be a oneliner dependency below:

jdbcTestImplementation project(':extensions-flight-sql')

I don't think java-test-fixture is necessarily the correct model... that said, I've divorced the jdbcTest source set from the test source set. There may be better ways to do this... but I think what I have is functionally correct. Also added some comments about the differences between test and jdbcTest.

...nsions/flight-sql/src/jdbcTest/java/io/deephaven/server/flightsql/FlightSqlJdbcTestBase.java

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlActionHelper.java

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

niloc132 · 2024-11-11T19:18:06Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+        // This general interface does not make sense; resolution should always be done against a _ticket_. Nothing
+        // calls io.deephaven.server.session.TicketRouter.resolve(SessionState, FlightDescriptor, String)


Should we deprecate/remove it? Did we previously have a plan for it that didn't pan out or otherwise was unnecessary/untenable?

I'd be happy to remove it here or in follow-up, cc @nbauernfeind

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

niloc132 · 2024-11-11T19:28:30Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+        {
+            final QueryScope scope = ExecutionContext.getContext().getQueryScope();
+            try {
+                obj = scope.readParamValue(table);
+            } catch (QueryScope.MissingVariableException e) {
+                return false;
+            }
+        }


Why the nested block? The only local that gets scoped into here is, err, scope, is there a downside of it "escaping" into the method scope?

I have been leaning into scoping things more explicitly; I find it helps structure code, esp code that is on the longer side. That said, pretty minor here, so will undo.

niloc132 · 2024-11-11T19:53:49Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+            }
+        };
+        for (int i = 0; i < L; ++i) {
+            final char c = flightSqlPattern.charAt(i);


...in addition to never having underscores, does this mean tables can never have unicode, or are _ and % not allowed as a later code unit in a codepoint? My unicode memory is not retained well.

At least right now, the table names are limited to the sorts of names we allow in the query scope.

That includes oodles of unicode, no? https://stackoverflow.com/a/17043983/860630

niloc132 · 2024-11-11T19:54:44Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+     * against, which is a common pattern used in Deephaven table names. As mentioned below, it also follows that an
+     * empty string should only explicitly match against an empty string.
+     */
+    private static Predicate<String> flightSqlFilterPredicate(String flightSqlPattern) {


I think this is worth testing, so we can explicitly call out edge cases in the test code, show what isn't expected to work (empty never matches empty, underscore matches underscore and also other things, etc), plus be sure we don't have any other surprises lurking

Added some explicit testing.

niloc132 · 2024-11-11T19:56:59Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlTicketHelper.java

+        if (ticket.get() != TICKET_PREFIX) {
+            // If we get here, it means there is an error with FlightSqlResolver.ticketRoute /
+            // io.deephaven.server.session.TicketRouter.getResolver
+            throw new IllegalStateException("Could not resolve Flight SQL ticket '" + logId + "': invalid prefix");


do we want to print the ticket itself too? e.g. ScopeTicketResolver.nameForTicket uses ByteHelper.byteBufToHex(ticket)

I'm less inclined in this specific case, as it represents an internal server error.

And more generally, I'm less inclined with FlightSql tickets specifically b/c they are arbitrary length Any messages.

niloc132 · 2024-11-11T19:58:22Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/TableCreatorScopeTickets.java

+
+    @Override
+    public Table of(TicketTable ticketTable) {
+        // This does not wrap in a nugget like TicketRouter.resolve; is that important?


is this a pre-merge todo?

It was a TODO to myself, but has been handled by a higher layer nugget now. Will remove comment.

niloc132 · 2024-11-11T22:29:17Z

server/jetty/build.gradle

+    compileOnlyApi(project(':util-thread')) {
+        because 'downstream dagger compile'
+    }


Thank you - I ran into this the other day. Want me to submit this in its own PR?

I'm not too worried; if this merges in soon, should be ok here.

niloc132 · 2024-11-11T23:08:41Z

server/src/main/java/io/deephaven/server/session/TicketRouter.java

+            throw e;
+            // return SessionState.wrapAsFailedExport(e);


Link with a TODO to the issue?

server/test-utils/src/main/java/io/deephaven/server/runner/DeephavenApiServerTestBase.java

niloc132 · 2024-11-12T15:32:16Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+        if (!(obj instanceof Table)) {
+            return false;
+        }
+        return authorization.transform((Table) obj) != null;


Maybe, but the call tree into here just to get a "true" or "false" back could make GetDbSchemas, GetTables, GetTableTypes etc quite expensive - and if the table is dropped at the end of the call when the liveness scope ends, the very next flightsql call will need to recompute it. Waiting seconds or minutes for each call and paying the same thing for the next call seems bad.

It sounds like Authorization might need something other than transform if we're going to use it this way? Transform is supposed to be the step just before using the table, not deciding if it can be used. Maybe a default method in the interface that just checks if the result of transform is null, and implementations can specify better behavior if they want to?

niloc132 · 2024-11-12T15:57:09Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+     *    - "_" means to match any one character.
+     * </pre>
+     *
+     * There does not seem to be any potential for escaping, which means that underscores can't explicitly be matched


Sorry, not meaning to be critical of the code itself (except the char-by-char thing, i think you want to iterate codepoints, or just scan next index of @/_), but the premise that flightsql is starting with. If tooling out there will use it and expects it to work a certain way, we should support it.

But this would have been so easy to represent for arbitrary tokens, so each lang/runtime could define its own quote and wildcard mechanism, something like

message Pattern { repeated Token tokens = 1; } message Token { oneof value { Wildcard wildcard = 1; string literal = 2; } } enum Wildcard { NOT_SET = 0; ONE = 1; AT_LEAST_ONE = 2; ANY = 3; }

niloc132 · 2024-11-12T16:06:46Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+            }
+        };
+        for (int i = 0; i < L; ++i) {
+            final char c = flightSqlPattern.charAt(i);


That includes oodles of unicode, no? https://stackoverflow.com/a/17043983/860630

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

niloc132 · 2024-11-13T02:03:10Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+        public final TicketHandlerReleasable initialize(C command) {
+            try {
+                return initializeImpl(command);
+            } catch (Throwable t) {
+                release();
+                throw t;
+            }
+        }
+
+        private synchronized QueryBase<C> initializeImpl(C command) {


Why are these called initialize when they wrap the actual call to execute() (granted, which itself has to delegate further to executeImpl)?

The naming seems to suggest that initialization happens before execution, but really initialization is execution (plus a guard for exceptions, calling more than once, and scheduling cleanup).

Yeah, I iterated on different naming. I will update the interface to "execute".

niloc132 · 2024-11-13T02:06:03Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+     * initialize.
+     */
+    static abstract class CommandHandlerFixedBase<T extends Message> implements CommandHandler<T> {
+        private final Class<T> clazz;


field is unused - but we require it to be non-null, and require subtypes to pass a type that ostensibly matches it?

niloc132 · 2024-11-13T02:08:43Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+            execute(command);
+            if (table == null) {
+                throw new IllegalStateException(
+                        "QueryBase implementation has a bug, should have set table");


arguable QueryBase should be getClass(), as it was the impl's execute() that failed to call executeImpl?

Also do we want to use Assert for these sorts of "we broke our own contract" assertions?

Updated to Assert.

niloc132 · 2024-11-13T02:34:24Z

extensions/flight-sql/src/main/java/io/deephaven/server/flightsql/FlightSqlResolver.java

+        }
+
+        private void onSessionClosed() {
+            log.debug().append("onSessionClosed: removing prepared statement ").append(handleId.toString()).endl();


nitpicky, so ignore if you want (but I think this might be the kind of thing you'd flag if you spotted it):
This is a little gross since this is the utf8 of a uuid string (36 bytes), and we pay for the toString() of the ByteString whether or not we log the debug...

Technically I think handleId.asReadOnlyByteBuffer would at least not copy it every time, and the bytebuffer is holding ascii instead of the actual bytes of the uuid, so that would print nicely too.

But do we make handleId by design to hold ascii? Could use UuidCreator.toBytes(UUID) and have 16 real bytes instead of 8 smeared across the LSBs of 32ish bytes? At that point you might consider just using SecureRandom.nextBytes(16) since we don't actually use the uuid api at all past this point.

Also worth noting that the length-36 ByteStrings going to get hashed by multiplying each by 31 and adding the next... my math isn't great here, but packing so many zeros isn't going to make for nice hashes right. Won't matter except under load, but then it might matter if we are collision heavy.

Could easily wait until it does matter, but it seems cheap to either extract the bytes (or generate the bytes) and live with ByteString's toString and wrap in an if debug check?

Updated in latest push.

jmao-denver and others added 15 commits July 29, 2024 16:39

Initial proj setup and minimal impl

185fd4f

Implement some meta calls

04288bf

Fix/disable tests

e79c493

Add JDBC test

2002936

Plumb rudamentary DoAction / GetFlightInfo command support

84c737b

Merge remote-tracking branch 'upstream/main' into 5339-flight-sql

f7c9df1

Ensure ticket tables go through auth transform

3c4ed60

Merge remote-tracking branch 'upstream/main' into 5339-flight-sql

d045ac4

Cleanup FlightSQL parts

770956d

cleanup auth cookie handling

51e039d

Disable gRPC trace logging

a04f8ab

Fix downstream dagger compile

9caef38

More cleanup

329bdbf

remove unused class

7d00fe6

more unused

7b79e70

devinrsmith added grpc DocumentationNeeded ReleaseNotesNeeded Release notes are needed sql labels Sep 5, 2024

devinrsmith added this to the 0.37.0 milestone Sep 5, 2024

devinrsmith requested review from niloc132 and nbauernfeind September 5, 2024 22:25

devinrsmith self-assigned this Sep 5, 2024

devinrsmith requested review from chipkent, jmao-denver and rcaudy as code owners September 5, 2024 22:25

chipkent previously approved these changes Sep 11, 2024

View reviewed changes

nbauernfeind reviewed Sep 23, 2024

View reviewed changes

devinrsmith added 2 commits September 23, 2024 13:41

Merge remote-tracking branch 'upstream/main' into 5339-flight-sql

90bb328

Add ActionResolver

9649204

devinrsmith requested a review from niloc132 October 29, 2024 18:53

devinrsmith added 4 commits October 29, 2024 16:01

Migrate flightsql to server-jetty-app

ca8e0b3

Migrate to extensions/flight-sql

89f0d70

fix EmbeddedServer

7d1f57b

add configuration to disable resolvers

938c05a

devinrsmith changed the title ~~feat: Add FlightSQL server support~~ feat: Add Flight SQL server support Oct 29, 2024

chipkent previously approved these changes Oct 30, 2024

View reviewed changes

devinrsmith added 2 commits November 4, 2024 11:13

Merge remote-tracking branch 'upstream/main' into 5339-flight-sql

094401f

Fixup util-thread wrt downstream dagger

49a0195

devinrsmith dismissed chipkent’s stale review via 49a0195 November 4, 2024 19:20

devinrsmith added 9 commits November 4, 2024 13:25

Ensure FlightSQL tests work on Java 17+

9830ce0

Only retain table if it is refreshing.

4ecbe2a

See comment in deephaven#4575

Small comments / error message changes

20c18d3

Add typed visitors for Commands and Tickets

260c679

Create FlightSqlActionHelper

00cd69e

Refactor constants

171e5b1

cleanup

fd4043a

Create ActionVisitorBase

bb77291

Create CommandVisitorBase

a902aef

niloc132 reviewed Nov 11, 2024

View reviewed changes

devinrsmith added 2 commits November 11, 2024 16:43

Review response

6ae2ea9

Divorce jdbcTest source set from test source set

306dc26

devinrsmith requested a review from niloc132 November 12, 2024 17:16

niloc132 reviewed Nov 13, 2024

View reviewed changes

devinrsmith mentioned this pull request Nov 13, 2024

FlightServiceGrpcImpl getFlightInfo / getSchema unauthenticated path misimplemented #6374

Open

devinrsmith requested a review from niloc132 November 13, 2024 18:11

devinrsmith added 3 commits November 13, 2024 10:12

review points

6a954f5

More review responses

758c42c

statementNeverExecuted and one more isDeniedAccess optimization

d9d4a8b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Flight SQL server support #6023

feat: Add Flight SQL server support #6023

devinrsmith commented Sep 5, 2024 •

edited

Loading

chipkent left a comment

chipkent left a comment

niloc132 left a comment

niloc132 Nov 11, 2024

devinrsmith Nov 12, 2024

niloc132 Nov 11, 2024

devinrsmith Nov 12, 2024

niloc132 Nov 11, 2024

devinrsmith Nov 12, 2024

niloc132 Nov 11, 2024

devinrsmith Nov 11, 2024

niloc132 Nov 12, 2024

niloc132 Nov 11, 2024

devinrsmith Nov 12, 2024

niloc132 Nov 11, 2024

devinrsmith Nov 13, 2024

devinrsmith Nov 13, 2024

niloc132 Nov 11, 2024

devinrsmith Nov 11, 2024

niloc132 Nov 11, 2024

devinrsmith Nov 12, 2024

niloc132 Nov 11, 2024

niloc132 Nov 12, 2024

niloc132 Nov 12, 2024

niloc132 Nov 12, 2024

niloc132 Nov 13, 2024

devinrsmith Nov 13, 2024

niloc132 Nov 13, 2024

devinrsmith Nov 13, 2024

niloc132 Nov 13, 2024

devinrsmith Nov 13, 2024

niloc132 Nov 13, 2024

devinrsmith Nov 13, 2024

		// This general interface does not make sense; resolution should always be done against a _ticket_. Nothing
		// calls io.deephaven.server.session.TicketRouter.resolve(SessionState, FlightDescriptor, String)

feat: Add Flight SQL server support #6023

Are you sure you want to change the base?

feat: Add Flight SQL server support #6023

Conversation

devinrsmith commented Sep 5, 2024 • edited Loading

chipkent left a comment

Choose a reason for hiding this comment

chipkent left a comment

Choose a reason for hiding this comment

niloc132 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devinrsmith commented Sep 5, 2024 •

edited

Loading