Conversation
|
I'm thinking we don't run-length-encode by default to simplify things further. |
e222af1 to
1a39168
Compare
CurtHagenlocher
left a comment
There was a problem hiding this comment.
Exciting! I've left a few comments and questions resulting from a first, casual review of the code.
| /// |--------------------------|------------------------------|----------| | ||
| /// | collection_name | utf8 not null | | | ||
| /// | collection_description | utf8 | | | ||
| /// | collection_schema | extension<arrow.schema_json> | | |
There was a problem hiding this comment.
The proposal for this type seems to have stalled :(.
There was a problem hiding this comment.
I'm talking to Kent, hopefully we can keep it going. Otherwise, I may propose an arrow.schema_ipc (even if that's a bit ugly)
There was a problem hiding this comment.
Having it not be an extension type would probably make it simpler (when there's a JSON option, regular arrow.json I think would suffice, given that the name of the field contains schema). Until then binary is probably fine, and IPC, though sort of gross, at least doesn't require most implementations to write an additional parser.
There was a problem hiding this comment.
Sort of off-topic then but I think it's kind of an implementation flaw that extension types are so hard to work with in many Arrow implementations 😅
Hmm, I suppose in practice the extension type is unnecessary for these fields. In theory I'm thinking that databow and other UIs around ADBC could use the extension type to know to render it in a more user-friendly way. In practice hardcoding it may be fine just because there's only a few cases/contexts where you're returning a schema in a table?
There was a problem hiding this comment.
The rendering is a good point, although if you use a functional-style matcher you can detect the parent struct based on the field names and print the whole thing nicely (probably better than you'd get by strictly keying on extension name).
a7ce389 to
2d08fe1
Compare
| /// defined by the collection. For example, a client may request a list of | ||
| /// tables in the database, or a list of supported data types. | ||
| /// | ||
| /// All drivers must implement a collection called "meta" (which is aliased to |
There was a problem hiding this comment.
| /// All drivers must implement a collection called "meta" (which is aliased to | |
| /// All drivers that implement this API must implement a collection called "meta" (which is aliased to |
There was a problem hiding this comment.
Isn't that kind of implicit?
There was a problem hiding this comment.
I don't think so but I'll defer to you on the language.
| /// \brief Filter the collection on the literal catalog name of the foreign | ||
| /// key. |
There was a problem hiding this comment.
This was hard to understand for me as written. Does this option let me filter to tables that are referenced by a table whose name I provide?
My confusion goes for this and the other options related to foreign keys. Maybe we mean something more like,
Filter the collection on the literal catalog name of the referencing table.
NB: I didn't catch this on my first read through but gpt-5.6-sol flagged it and I found I wasn't able to explain it one way or the other.
There was a problem hiding this comment.
It's ~directly ported from JDBC https://docs.oracle.com/javase/8/docs/api/java/sql/DatabaseMetaData.html#getCrossReference-java.lang.String-java.lang.String-java.lang.String-java.lang.String-java.lang.String-java.lang.String-
Though I noticed below I flubbed the filters list so will fix that
There was a problem hiding this comment.
The way I understood it, I thought it was an option to filter by external catalogs that are non-native (i.e foreign) to the platform. For example external Iceberg catalogs in Databricks, reading AWS Glue from Snowflake etc.
| /// use a vendor-specific prefix (e.g. `postgresql.`) to avoid conflicts with | ||
| /// future standardized collections. Drivers must not use the `adbc.` prefix. | ||
| /// | ||
| /// Drivers may not necessarily accept filter options or other options before |
There was a problem hiding this comment.
"May not necessarily accept" feels a bit ambiguous. It seems there's an implicit state machine here with regards to ADBC_METADATA_* options relative to ADBC_METADATA_COLLECTION both in the lifecycle and what the valid/invalid states should be. Should the spec make that more explicit?
There was a problem hiding this comment.
Yes, there's also bulk-ingest as part of the state machine.
I agree it needs to be made more explicit. If we're broadly OK with this approach to metadata, then I'll document it better.
| /// fixed. | ||
| /// | ||
| /// Drivers may implement collections beyond those defined by ADBC, but must | ||
| /// use a vendor-specific prefix (e.g. `postgresql.`) to avoid conflicts with |
There was a problem hiding this comment.
prefix field names with the vendor/driver name to differentiate them (e.g. 'POSTGRESQL:owner', not just 'owner').
vendor-specific prefix (e.g.
postgresql.)
Could these two suggestions be made consistent? Or is this intentional?
There was a problem hiding this comment.
I'd say that most of the time we prefer prefix. in ADBC, but for whatever reason when the metadata is in Arrow field/schema metadata we've gone with PREFIX: (a convention started upstream). It does feel arbitrary, though.
There was a problem hiding this comment.
Ah, it's actually specified upstream: https://arrow.apache.org/docs/format/Columnar.html#custom-application-metadata
| /// vendor-specific metadata. Applications must access these by name or using | ||
| /// an offset from the end of the schema and cannot assume that the index of | ||
| /// the field will remain stable. Hence, it is discouraged to access these | ||
| /// fields by ordinal as this is brittle. Drivers must add the fields at the |
There was a problem hiding this comment.
What's the value of mentioning offset-from-the-end here, as opposed to just specifying that field access must be by name?
It seems only stable for a single driver at a single version, since you'd need to know how many fields that driver appends and in what order within its block.
The paragraph also calls ordinal access brittle two sentences later, which seems to agree. Is there a scenario I'm missing?
There was a problem hiding this comment.
It's to clarify that you can't assume we won't add more fields in the spec, so if you do use an ordinal, then it needs to be from the end.
| /// \brief Filter the collection on the literal routine name. | ||
| /// | ||
| /// The type is char*. | ||
| #define ADBC_METADATA_FILTER_ROUTINE "adbc.metadata.filter.routine" |
There was a problem hiding this comment.
To make sure I understand this right: we're defining anything that can run as user-defined code as a "routine", so for example Snowflake has SQL UDFs, Snowpark UDFS (Python/JS) and Stored Procedures, and all of those would fall under the same "routine" category?
And the way to filter those out if I wanted only Python UDFs for example us by using the routine_types filter, correct? The values of those filters will be platform dependent, so for Snowflake it'd be sql_udf, python_udf, js_udf, stored_procedure. And each driver would need a list of platform-specific supported routine_types.
Is it worth to standardize common routine types, or do you think that's a little too much? For example stored_procedure is a pretty standard thing across a lot of different databases, so maybe it would be nice to have a shared key for all of those?
There was a problem hiding this comment.
Now that I think of it, maybe it's too early to standardize that... Databricks alone has like 3 different kinds of Python UDFs. And the definition of a stored_procedure is fuzzy...
| /// | constraint_deferrability | int16 | (4) | | ||
| /// | constraint_match_type | int16 | (5) | | ||
| /// | ||
| /// 1. The 1-based index of the column pair within the foreign key (1 => first |
There was a problem hiding this comment.
Is there a reason why we're using 1-based indices?
There was a problem hiding this comment.
In general, for most of your questions: we are directly copying JDBC/ODBC for these definitions for consistency.
| /// Some drivers may support this to allow fetching a large metadata | ||
| /// collection in multiple calls. If the pagination token could not be used, | ||
| /// the driver should return an error. | ||
| #define ADBC_METADATA_OPTION_PAGINATION_TOKEN "adbc.metadata.pagination_token" |
There was a problem hiding this comment.
Two things:
-
I assume this is an "internal" key that will be used by the driver manager implementations to return stateful
RecordBatchReaders that paginate under the hood when you pull a new batch, right? I think it would be nice if applications didn't need to worry about setting this option and keeping track of cursors across multiple calls. As an application I'd like to just consume record batches and the pagination cursors and state are handled by the driver manager. -
Is this intended to be used as both an input option as well as an output metadata key? If my understanding is correct, the lifecycle is:
- The app (driver manager) would call
AdbcConnectionGetInfowithout aadbc.metadata.pagination_token(the first call), then the driver returns a record batch with aadbc.metadata.pagination_tokenin the schema metadata. - The next call, the app passes in the same
adbc.metadata.pagination_token. I don't understand where, sinceAdbcConnectionGetInfohas no explicit support for options. Does this get set as a connection-level parameter that is set before the call and unset after it returns? Does this get added together with the filters? - This repeats until the last page, where the driver returns no
adbc.metadata.pagination_token, signifying it's done listing.
I think it would be nice to document this lifecycle somewhere. Not sure the right place for it is here, though.
There was a problem hiding this comment.
You don't need this in normal operation. The driver manager also isn't involved. This is meant to let you resume an operation across different fetches; a regular fetch should already paginate.
|
@zeroshade can I get a look here? 🙏 |
Related:
Closes #4400.