Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(format): add info codes for supported capabilities #1649

Merged
merged 12 commits into from
Apr 12, 2024
Merged

feat(format): add info codes for supported capabilities #1649

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
f13db15
feat(format): add info codes for supported capabilities
joellubi Mar 20, 2024
ce63632
add info_codes to adbc.go
joellubi Mar 21, 2024
6c8ce03
refactor and integrate new info codes with go driverbase and drivers
joellubi Mar 21, 2024
3f0b83e
driver_info for snowflake
joellubi Mar 21, 2024
f0e974d
reformat error message
joellubi Mar 21, 2024
d84055e
update error string in test
joellubi Mar 21, 2024
492c333
add java info codes
joellubi Mar 21, 2024
633e025
sync header file
joellubi Mar 21, 2024
aeb0fba
reformat java comment
joellubi Mar 21, 2024
81da8a1
move ingest option keys from options.h to adbc.h
joellubi Apr 1, 2024
197a470
add options to adbc.go
joellubi Apr 1, 2024
a52a4fa
remove readonly info code
joellubi Apr 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions adbc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -459,6 +459,28 @@ const struct AdbcError* AdbcErrorFromArrayStream(struct ArrowArrayStream* stream
///
/// \see AdbcConnectionGetInfo
#define ADBC_INFO_VENDOR_ARROW_VERSION 2
/// \brief Indicates whether the driver connection is read only (type: bool).
///
/// \see AdbcConnectionGetInfo
#define ADBC_INFO_VENDOR_READ_ONLY 3
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Should this be an actual option (GetOption/SetOption), since it in principle is mutable in other APIs? e.g. JDBC Connection#setReadOnly. Flight SQL of course doesn't let you set this, but it may apply to other things in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I actually don't need this particular option, it just seemed useful while I was adding the others. So perhaps it's a better idea to leave it out (and maybe add it to options.h). I can update this tomorrow.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just pushed this change. It turns out that adbc.h already has ADBC_CONNECTION_OPTION_READ_ONLY defined, so I didn't need to add anything to options.h.

Thanks for the review, it would've been a mistake to make this an info code.

/// \brief Indicates whether SQL queries are supported (type: bool).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also make a new value for ADBC_VERSION ? 1.2.0 for example?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's some discussion about this in #1650. What do you think about version 1.1.1?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean for the version returned in GetInfo, or also making a new constant that would be recognized by the driver manager and such?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I'm looking through the code now for references to the constant ADBC_VERSION_1_1_0, it does seem like a non-trivial amount of work to update the logic everywhere to account for ADBC_VERSION_1_1_1. Especially for such a minor and backward-compatible change, it doesn't seem worth it.

Perhaps we could just do a comment / documentation update here to denote the minor version bump? Also, a refactor of ADBC version handling might be helpful going forward if it would simplify the logic for various feature gates that would allow us to bump versions more seamlessly.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We basically treat the version as an ABI version. Adding constants doesn't change the API, so there's no need to bump.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or at least, that's my justification for not having to deal with that mess here ^_^

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think considering the ADBC version to be an ABI version makes sense. If a driver implementation were to adopt this "version" it would not require any changes in behavior, as there's no actual requirement to send or recognize the new constants.

I think it may actually be more confusing to describe this as a new version, since there are no discernible differences in behavior between 1.1.0 and 1.1.1.

Any objections to moving forward with this PR as it currently is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with that.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me too -- sorry for not responding sooner. Thank you @joellubi and @lidavidm

///
/// \see AdbcConnectionGetInfo
#define ADBC_INFO_VENDOR_SQL 4
/// \brief Indicates whether Substrait queries are supported (type: bool).
///
/// \see AdbcConnectionGetInfo
#define ADBC_INFO_VENDOR_SUBSTRAIT 5
/// \brief The minimum supported Substrait version, or null if
/// Substrait is not supported (type: utf8).
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would using a type other than utf8 for a version make sense?

The ADBC version appears to have a numeric encoding:

/// \since ADBC API revision 1.1.0
#define ADBC_VERSION_1_1_0 1001000

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a numeric encoding would have some benefits, but in this case utf8 is most consistent with existing encodings of substrait version.

For context the version field of SubstraitPlan is a string in FlightSql.proto. In substrait itself Version is a message that can either use numeric major.minor.patch versions or simply be a git_hash, so string may be required.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a string makes sense to me then 👍

///
/// \see AdbcConnectionGetInfo
#define ADBC_INFO_VENDOR_SUBSTRAIT_MIN_VERSION 6
/// \brief The maximum supported Substrait version, or null if
/// Substrait is not supported (type: utf8).
///
/// \see AdbcConnectionGetInfo
#define ADBC_INFO_VENDOR_SUBSTRAIT_MAX_VERSION 7

/// \brief The driver name (type: utf8).
///
Expand Down Expand Up @@ -754,6 +776,24 @@ const struct AdbcError* AdbcErrorFromArrayStream(struct ArrowArrayStream* stream
/// schema of the data to append (ADBC_STATUS_ALREADY_EXISTS).
/// \since ADBC API revision 1.1.0
#define ADBC_INGEST_OPTION_MODE_CREATE_APPEND "adbc.ingest.mode.create_append"
/// \brief The catalog of the table for bulk insert.
///
/// The type is char*.
#define ADBC_INGEST_OPTION_TARGET_CATALOG "adbc.ingest.target_catalog"
/// \brief The schema of the table for bulk insert.
///
/// The type is char*.
#define ADBC_INGEST_OPTION_TARGET_DB_SCHEMA "adbc.ingest.target_db_schema"
/// \brief Use a temporary table for ingestion.
///
/// The value should be ADBC_OPTION_VALUE_ENABLED or
/// ADBC_OPTION_VALUE_DISABLED (the default).
///
/// This is not supported with ADBC_INGEST_OPTION_TARGET_CATALOG and
/// ADBC_INGEST_OPTION_TARGET_DB_SCHEMA.
///
/// The type is char*.
#define ADBC_INGEST_OPTION_TEMPORARY "adbc.ingest.temporary"

/// @}

Expand Down
21 changes: 0 additions & 21 deletions c/driver/common/options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,27 +24,6 @@
extern "C" {
#endif

/// \brief The catalog of the table for bulk insert.
///
/// The type is char*.
#define ADBC_INGEST_OPTION_TARGET_CATALOG "adbc.ingest.target_catalog"

/// \brief The schema of the table for bulk insert.
///
/// The type is char*.
#define ADBC_INGEST_OPTION_TARGET_DB_SCHEMA "adbc.ingest.target_db_schema"

/// \brief Use a temporary table for ingestion.
///
/// The value should be ADBC_OPTION_VALUE_ENABLED or
/// ADBC_OPTION_VALUE_DISABLED (the default).
///
/// This is not supported with ADBC_INGEST_OPTION_TARGET_CATALOG and
/// ADBC_INGEST_OPTION_TARGET_DB_SCHEMA.
///
/// The type is char*.
#define ADBC_INGEST_OPTION_TEMPORARY "adbc.ingest.temporary"

#ifdef __cplusplus
}
#endif
14 changes: 13 additions & 1 deletion go/adbc/adbc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,9 @@ const (
OptionValueIngestModeAppend = "adbc.ingest.mode.append"
OptionValueIngestModeReplace = "adbc.ingest.mode.replace"
OptionValueIngestModeCreateAppend = "adbc.ingest.mode.create_append"
OptionValueIngestTargetCatalog = "adbc.ingest.target_catalog"
OptionValueIngestTargetDBSchema = "adbc.ingest.target_db_schema"
OptionValueIngestTemporary = "adbc.ingest.temporary"
OptionKeyURI = "uri"
OptionKeyUsername = "username"
OptionKeyPassword = "password"
Expand Down Expand Up @@ -344,7 +347,16 @@ const (
InfoVendorVersion InfoCode = 1 // VendorVersion
// The database vendor/product Arrow library version (type: utf8)
InfoVendorArrowVersion InfoCode = 2 // VendorArrowVersion

// Indicates whether the driver connection is read only (type: bool).
InfoVendorReadOnly InfoCode = 3
// Indicates whether SQL queries are supported (type: bool).
InfoVendorSql InfoCode = 4
// Indicates whether Substrait queries are supported (type: bool).
InfoVendorSubstrait InfoCode = 5
// The minimum supported Substrait version, or null if Substrait is not supported (type: utf8).
InfoVendorSubstraitMinVersion InfoCode = 6
// The maximum supported Substrait version, or null if Substrait is not supported (type: utf8).
InfoVendorSubstraitMaxVersion InfoCode = 7
// The driver name (type: utf8)
InfoDriverName InfoCode = 100 // DriverName
// The driver version (type: utf8)
Expand Down
2 changes: 2 additions & 0 deletions go/adbc/driver/flightsql/flightsql_adbc_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,8 @@ func (s *FlightSQLQuirks) GetMetadata(code adbc.InfoCode) interface{} {
return "sqlite 3"
case adbc.InfoVendorArrowVersion:
return "16.0.0-SNAPSHOT"
case adbc.InfoVendorReadOnly:
return false
}

return nil
Expand Down
51 changes: 36 additions & 15 deletions go/adbc/driver/flightsql/flightsql_connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,9 +134,14 @@ func (c *connectionImpl) SetAutocommit(enabled bool) error {
}

var adbcToFlightSQLInfo = map[adbc.InfoCode]flightsql.SqlInfo{
adbc.InfoVendorName: flightsql.SqlInfoFlightSqlServerName,
adbc.InfoVendorVersion: flightsql.SqlInfoFlightSqlServerVersion,
adbc.InfoVendorArrowVersion: flightsql.SqlInfoFlightSqlServerArrowVersion,
adbc.InfoVendorName: flightsql.SqlInfoFlightSqlServerName,
adbc.InfoVendorVersion: flightsql.SqlInfoFlightSqlServerVersion,
adbc.InfoVendorArrowVersion: flightsql.SqlInfoFlightSqlServerArrowVersion,
adbc.InfoVendorReadOnly: flightsql.SqlInfoFlightSqlServerReadOnly,
adbc.InfoVendorSql: flightsql.SqlInfoFlightSqlServerSql,
adbc.InfoVendorSubstrait: flightsql.SqlInfoFlightSqlServerSubstrait,
adbc.InfoVendorSubstraitMinVersion: flightsql.SqlInfoFlightSqlServerSubstraitMinVersion,
adbc.InfoVendorSubstraitMaxVersion: flightsql.SqlInfoFlightSqlServerSubstraitMaxVersion,
}

func doGet(ctx context.Context, cl *flightsql.Client, endpoint *flight.FlightEndpoint, clientCache gcache.Cache, opts ...grpc.CallOption) (rdr *flight.Reader, err error) {
Expand Down Expand Up @@ -564,21 +569,37 @@ func (c *connectionImpl) PrepareDriverInfo(ctx context.Context, infoCodes []adbc

var adbcInfoCode adbc.InfoCode
for i := 0; i < int(rec.NumRows()); i++ {
switch flightsql.SqlInfo(field.Value(i)) {
case flightsql.SqlInfoFlightSqlServerName:
adbcInfoCode = adbc.InfoVendorName
case flightsql.SqlInfoFlightSqlServerVersion:
adbcInfoCode = adbc.InfoVendorVersion
case flightsql.SqlInfoFlightSqlServerArrowVersion:
adbcInfoCode = adbc.InfoVendorArrowVersion
default:

var found bool
idx := int(info.ValueOffset(i))
flightSqlInfoCode := flightsql.SqlInfo(field.Value(i))
for infocode := range adbcToFlightSQLInfo {
if adbcToFlightSQLInfo[infocode] == flightSqlInfoCode {
adbcInfoCode = infocode
found = true
break
}
}

// SqlInfo on the server that does not have an explicit mapping to ADBC is ignored
if !found {
continue
}

// we know we're only doing string fields here right now
v := info.Field(info.ChildID(i)).(*array.String).
Value(int(info.ValueOffset(i)))
if err := driverInfo.RegisterInfoCode(adbcInfoCode, strings.Clone(v)); err != nil {
var v any
switch arr := info.Field(info.ChildID(i)).(type) {
case *array.String:
v = strings.Clone(arr.Value(idx))
case *array.Boolean:
v = arr.Value(idx)
default:
return adbc.Error{
Msg: fmt.Sprintf("unsupported field_type %T for info_value", arr),
Code: adbc.StatusInvalidArgument,
}
}

if err := driverInfo.RegisterInfoCode(adbcInfoCode, v); err != nil {
return err
}
}
Expand Down
6 changes: 2 additions & 4 deletions go/adbc/driver/flightsql/flightsql_database.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -383,10 +383,8 @@ func getFlightClient(ctx context.Context, loc string, d *databaseImpl, authMiddl
target = "unix:" + uri.Path
}

driverVersion, ok := d.DatabaseImplBase.DriverInfo.GetInfoDriverVersion()
if !ok {
driverVersion = driverbase.UnknownVersion
}
dv, _ := d.DatabaseImplBase.DriverInfo.GetInfoForInfoCode(adbc.InfoDriverVersion)
driverVersion := dv.(string)
dialOpts := append(d.dialOpts.opts, grpc.WithConnectParams(d.timeout.connectParams()), grpc.WithTransportCredentials(creds), grpc.WithUserAgent("ADBC Flight SQL Driver "+driverVersion))

d.Logger.DebugContext(ctx, "new client", "location", loc)
Expand Down
80 changes: 29 additions & 51 deletions go/adbc/driver/internal/driverbase/connection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,65 +150,43 @@ func (base *ConnectionImplBase) GetInfo(ctx context.Context, infoCodes []adbc.In
infoValueBldr := bldr.Field(1).(*array.DenseUnionBuilder)
strInfoBldr := infoValueBldr.Child(int(adbc.InfoValueStringType)).(*array.StringBuilder)
intInfoBldr := infoValueBldr.Child(int(adbc.InfoValueInt64Type)).(*array.Int64Builder)
boolInfoBldr := infoValueBldr.Child(int(adbc.InfoValueBooleanType)).(*array.BooleanBuilder)

for _, code := range infoCodes {
switch code {
case adbc.InfoDriverName:
name, ok := base.DriverInfo.GetInfoDriverName()
if !ok {
continue
}

infoNameBldr.Append(uint32(code))
infoValueBldr.Append(adbc.InfoValueStringType)
strInfoBldr.Append(name)
case adbc.InfoDriverVersion:
version, ok := base.DriverInfo.GetInfoDriverVersion()
if !ok {
continue
}

infoNameBldr.Append(uint32(code))
infoValueBldr.Append(adbc.InfoValueStringType)
strInfoBldr.Append(version)
case adbc.InfoDriverArrowVersion:
arrowVersion, ok := base.DriverInfo.GetInfoDriverArrowVersion()
if !ok {
continue
}
infoNameBldr.Append(uint32(code))
value, ok := base.DriverInfo.GetInfoForInfoCode(code)

// We want to return a null value if the info_code requested is set to nil.
// The null value needs a type so we arbitrarily choose string (type_code: 0)
if value == nil {
value = ""
ok = false
}

infoNameBldr.Append(uint32(code))
switch v := value.(type) {
case string:
infoValueBldr.Append(adbc.InfoValueStringType)
strInfoBldr.Append(arrowVersion)
case adbc.InfoDriverADBCVersion:
adbcVersion, ok := base.DriverInfo.GetInfoDriverADBCVersion()
if !ok {
continue
if ok {
strInfoBldr.Append(v)
} else {
strInfoBldr.AppendNull()
}

infoNameBldr.Append(uint32(code))
case int64:
infoValueBldr.Append(adbc.InfoValueInt64Type)
intInfoBldr.Append(adbcVersion)
case adbc.InfoVendorName:
name, ok := base.DriverInfo.GetInfoVendorName()
if !ok {
continue
if ok {
intInfoBldr.Append(v)
} else {
intInfoBldr.AppendNull()
}

infoNameBldr.Append(uint32(code))
infoValueBldr.Append(adbc.InfoValueStringType)
strInfoBldr.Append(name)
default:
infoNameBldr.Append(uint32(code))
value, ok := base.DriverInfo.GetInfoForInfoCode(code)
if !ok {
infoValueBldr.AppendNull()
continue
case bool:
infoValueBldr.Append(adbc.InfoValueBooleanType)
if ok {
boolInfoBldr.Append(v)
} else {
boolInfoBldr.AppendNull()
}

// TODO: Handle other custom info types
infoValueBldr.Append(adbc.InfoValueStringType)
strInfoBldr.Append(fmt.Sprint(value))
default:
return nil, fmt.Errorf("no defined type code for info_value of type %T", v)
}
}

Expand Down
Loading
Loading