go/adbc/driver/flightsql: support generic ingest

[joellubi]

I wanted to pose the question of what it would take to be able to support ingest with the flightsql driver. I understand that each driver is meant to supply its own specific implementation for ingestion, which makes doing so for a flightsql backend challenging because the driver wouldn't necessarily know the specifics of it's underlying representation or syntax.

I had a few thoughts on how this might be achieved:

• One option would be to extend the flightsql spec to support ingest natively. This would enable any backend to implement its own ingest logic, and the adbc ingest would simply map to the flightsql ingest. This could be done by adding a new command message type to include in the FlightDescriptor of a DoPut call.
• Another option, though perhaps longer term and less clear, would be to defer this to substrait. The problem is that a generic flightsql adbc driver would not know the UPDATE or INSERT or COPY syntax to submit as a query to the backend, but perhaps a substrait plan could abstract the "ingestion plan". Now I'm not very familiar with the details of the substrait spec and I've only seen it used for "SELECT" style queries, so this may not even align with its stated goals. I think the first option is likely a better fit.

Elaborating on how the first option might be implemented, here's an example of how the new message type might look:

message CommandStatementIngest {
  option (experimental) = true;

  enum IngestMode {
    INGEST_MODE_CREATE = 0;
    INGEST_MODE_APPEND = 1;
    INGEST_MODE_REPLACE = 2;
    INGEST_MODE_CREATE_APPEND = 3;
  }

  string target_table = 1;

  IngestMode mode = 2;
}

After receiving this in the FlightDescriptor, the flightsql server may then handle the subsequent stream with whichever means provide the desired throughput to the requested target.

I would appreciate any feedback on this approach, or links to prior context I may have missed. Thanks!

[lidavidm]

Substrait can handle UPDATE etc. but I am not sure if any implementations support it.

Adding support to Flight SQL may be useful, a better place to discuss would be the Arrow dev@ mailing list

[lidavidm]

Thanks for the post. I think this would be useful to support in Flight SQL. I'd propose some slight changes:

message CommandStatementIngest {
  option (experimental) = true;

  enum IngestMode {
    INGEST_MODE_UNSPECIFIED = 0;
    INGEST_MODE_CREATE = 1;
    INGEST_MODE_APPEND = 2;
    INGEST_MODE_REPLACE = 3;
    INGEST_MODE_CREATE_APPEND = 4;
  }

  IngestMode mode = 1;
  string target_table = 1;
  optional string target_schema = 2;
  optional string target_catalog = 3;
  // do we want db-specific parameters?
}

[joellubi]

Thanks for the suggestion, it makes sense to include those fields.

Regarding your question about db-specific parameters, I can see the following options:

1. Add each option explicitly to the spec, but this would probably only work for parameters that generalize across many backends (i.e. not db-specific).
2. Add a single field capable of storing arbitrary key-value pairs, like app_metadata.
3. Use the existing app_metadata field of the initial FlightData sent as part of DoPut to include db-specific kv pairs.

I would appreciate your perspective on these options (or any others) @lidavidm.

[alamb]

Thank you @lidavidm and @joellubi -- I also think this basic idea makes sense.

In terms of the modes, SQL systems I am familiar with typically support two types, which are typically dialect specific and fairly complex:

1. INSERT/COPY: appends the new rows to the target table
2. UPSERT/MERGE potentially updates existing rows if present, and inserts new rows if not (for example, snowflake)

In order to implement UPSERT / MERGE you typically need to specify the criteria of what qualifies as an update. For some systems this is done via a declaration of PRIAMARY KEY but many others you can also potentially specify a custom matching condition

Thus I suggest supporting bulk insert in a generic way via a SQL query rather than an enum and table names, which would constrain how this feature gets used.

Perhaps we could add something like the follow (to mirror Update). It would likely make sense to have a prepared statement version of this as well

/*
 * Represents a SQL bulk insert / upsert query. Used in the command member of FlightDescriptor
 * for the the RPC call DoPut to cause the server to execute the included SQL INSERT/COPY/UPSERT/MERGE or similar
 * command, with the data in the batches in the DoPut call.
 */
message CommandStatementInsert {
  option (experimental) = true;

  // The SQL syntax.
  string query = 1;
  // Include the query as part of this transaction (if unset, the query is auto-committed).
  optional bytes transaction_id = 2;
}

[lidavidm]

I think the whole idea is to avoid having to know the specific SQL query (and I believe this is less for upsert and more for plain insert/copy; ADBC doesn't handle updates for its version of this). You are right we should include the transaction field, though.

[alamb]

🤔 since Insert / merge are pretty specific to the system and not standardized the SQL does vary quite a bit. Maybe in this case there is no better option than to support the lowest common denominator in terms of functionality 🤔

What is the intended semantics of CREATE by the way? Is it meant to create a new table it it doesn't already exist?

If so, I wonder how that would work with SQL types, which are not typically the same as Arrow types (e.g. a SQL VARCHAR might be a Utf8Array or a LargeUtf8Array). Is the idea that each FlightSQL implementation would automatically provide a mapping to the types, and if users wanted more control (like defining specific types or keys, etc) the would use the native DDL commands (CREATE TABLE ...)?

[lidavidm]

Yup, CREATE is 'create a new table, fail if already exists'.

Yes, that's pretty much what ADBC does here - the driver/server does its best to map the Arrow types to reasonable database types. If you need full control you'll have to use SQL yourself.

Substrait could provide some generic functionality but it's not well supported yet.

[lidavidm]

Add a single field capable of storing arbitrary key-value pairs, like app_metadata.

@joellubi this would be my vote (just a bytes field, or possibly a bytes field + map<string, string>)

[joellubi]

just a bytes field, or possibly a bytes field + map<string, string>

@lidavidm Could you please elaborate on what you mean by the latter option? Do you mean a bytes field encoding a map<string, string> or to actually use the native proto map field?

[lidavidm]

the latter, because it would presumably be easier for most things; but we've stuck to bytes fields elsewhere in Flight

[joellubi]

Got it. I agree it would be nice to use map though I'm unfamiliar if there are any prior decisions or precedent that might make bytes preferable.

If we went with map then this would be the current draft:

message CommandStatementIngest {
  option (experimental) = true;

  enum IngestMode {
    INGEST_MODE_UNSPECIFIED = 0;
    INGEST_MODE_CREATE = 1;
    INGEST_MODE_APPEND = 2;
    INGEST_MODE_REPLACE = 3;
    INGEST_MODE_CREATE_APPEND = 4;
  }

  IngestMode mode = 1;
  string target_table = 2;
  optional string target_schema = 3;
  optional string target_catalog = 4;
  map<string, string> options = 5;
}

Open to suggestions on the name for that field. app_metadata is used in other messages but it might be unintuitive to call it the same thing if it's not a bytes type.

[lidavidm]

@zeroshade @ywc88 comments here?

[judahrand]

Should we also incorporate the temporary flag which ADBC uses?

message CommandStatementIngest {
  option (experimental) = true;

  enum IngestMode {
    INGEST_MODE_UNSPECIFIED = 0;
    INGEST_MODE_CREATE = 1;
    INGEST_MODE_APPEND = 2;
    INGEST_MODE_REPLACE = 3;
    INGEST_MODE_CREATE_APPEND = 4;
  }

  IngestMode mode = 1;
  string target_table = 2;
  optional string target_schema = 3;
  optional string target_catalog = 4;
  optional bool temporary = 5;
  map<string, string> options = 1000;
}

[lidavidm]

Ah yes, good catch.