feat(format): add generic metadata API - #4694
Conversation
|
I'm thinking we don't run-length-encode by default to simplify things further. |
e222af1 to
1a39168
Compare
CurtHagenlocher
left a comment
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).
|
|
||
| /// \brief The "meta" collection returns the available metadata collections. | ||
| /// | ||
| /// | Field Name | Field Type | Comments | |
There was a problem hiding this comment.
Do we need to say that the field types can be logical vs physical e.g. REE- or dictionary-encoded? Or is that always implicit? EDIT: I just saw that the consumer can explicitly ask for this kind of encoding if that's what they want. This doesn't necessarily imply that the producer is otherwise constrained to provide e.g. utf8 instead of utf8_view so I still think it would be value to express that explicitly if it's the case.
There was a problem hiding this comment.
I think we should require drivers to stick to the declared type unless requested, I can clarify this
| /// | ||
| /// Drivers may add more fields at the end of standard schemas to reflect | ||
| /// vendor-specific metadata. Applications must access these using an offset | ||
| /// from the end of the schema and cannot assume that the index of the field |
There was a problem hiding this comment.
Do we want to generally discourage access by ordinal?
There was a problem hiding this comment.
I updated the text to specify access by name.
| /// - ADBC_METADATA_FILTER_PATTERN_COLUMN | ||
| #define ADBC_METADATA_COLLECTION_ROUTINE_COLUMNS "routine_columns" | ||
|
|
||
| /// \brief The "namespaces" collection returns a level of namespaces defined |
There was a problem hiding this comment.
These don't seem to interact much with the other metadata, or at least I'm having trouble seeing how they would. Do you have an example of a scenario where these matter, and how they would impact e.g. the listing of tables?
There was a problem hiding this comment.
Ah, I can take this out...I was trying to explore how things might work for databases that don't follow the catalog-schema-table hierarchy, especially now that Iceberg/Iceberg REST Catalog has introduced arbitrarily-nested namespaces into systems. But as mentioned I think we would basically have to duplicate all the metadata collections to support that and it's not worth trying to make them both fit into the same collections.
a7ce389 to
2d08fe1
Compare
| /// This is intended to replace AdbcConnectionGetObjects, but both APIs will | ||
| /// be supported for the time being. AdbcConnectionGetObjects may be | ||
| /// deprecated in a future revision. |
There was a problem hiding this comment.
Kind of burying the lede here. Do you think it would be helpful if this was right at the top of the defgroup doc? And maybe this PR could add a comment on AdbcConnectionGetObjects about the potential future for deprecation.
There was a problem hiding this comment.
To add: What should I do if I'm starting a new diver project? Implement both GetObjects and this? I'd think that's what we want so we could say it clearly here.
There was a problem hiding this comment.
I think for someone coming in to ADBC, we don't need to draw the connection to GetObjects right away. I don't want to define things in terms of other things that will be deprecated. I'll clarify that we expect both to be implemented going forward for compatibility.
| /// option wins. | ||
| /// | ||
| /// The type is char*. | ||
| #define ADBC_METADATA_FILTER_PATERN_DB_SCHEMA "adbc.metadata.filter_pattern.schema" |
There was a problem hiding this comment.
Typo:
| #define ADBC_METADATA_FILTER_PATERN_DB_SCHEMA "adbc.metadata.filter_pattern.schema" | |
| #define ADBC_METADATA_FILTER_PATTERN_DB_SCHEMA "adbc.metadata.filter_pattern.schema" |
| /// Drivers may also implement AdbcStatementRequestSchema to (1) request | ||
| /// different data types and (2) drop fields from the result. Drivers are not |
There was a problem hiding this comment.
Hrm. The current doc comment on AdbcStatementRequestSchema says,
column reordering or changing the number of returned columns
is not a goal of this feature.
Could what say here in this PR be confusing given that?
There was a problem hiding this comment.
At the same time, I don't want to define a separate API. I'll clarify that RequestSchema isn't meant to stand in for projection of a database query's result set.
| /// 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.
I'm not sure this is right but:
| /// \brief Filter the collection on the literal catalog name of the foreign | |
| /// key. | |
| /// \brief Filter the collection on the literal schema name of the foreign | |
| /// key. |
| /// \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.
| /// | Field Name | Field Type | Comments | | ||
| /// |--------------------------|------------------------------|----------| | ||
| /// | collection_name | utf8 not null | | | ||
| /// | collection_description | utf8 | | |
There was a problem hiding this comment.
Minor: As a driver author, I'm not sure I'd know what to put in here.
| /// | filter_description | utf8 | | | ||
| /// | required | bool not null | | |
There was a problem hiding this comment.
How can the client act on collection_filters given this schema?
Say I query the meta collection and see it has a tables member and collection_filters is non-zero. Now I know I can list tables with a filter but how does collection_filters help me filter?
There was a problem hiding this comment.
Ah, I should add more fields here now that we're not using positional arguments
There was a problem hiding this comment.
Updated to be more like DriverGetOptionsList
| /// 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
| /// database. | ||
| /// | ||
| /// Some systems may not have the concept of catalogs, in which case this | ||
| /// collection should contain a single entry with an empty, non-null name. |
There was a problem hiding this comment.
in which case this collection should contain a single entry with an empty, non-null name
This seems in tension with the purpose of the meta collection. It seems more consistent to have this be unimplemented, and to not return catalogs from meta if a database has no concept of a catalog.
There was a problem hiding this comment.
I wonder what JDBC/ODBC drivers do in this case. I'll check.
There was a problem hiding this comment.
The precedent is to implement it but just return an empty collection:
Let's be consistent with that.
| /// 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.
6 participants
Related:
Closes #4400.