zeroshade commented on code in PR #893:
URL: https://github.com/apache/arrow-adbc/pull/893#discussion_r1261279505
##########
go/adbc/drivermgr/adbc.h:
##########
@@ -334,25 +334,35 @@ struct ADBC_EXPORT AdbcError {
/// Buffers) that can be optionally parsed by clients, beyond the
/// standard AdbcError fields, without having to encode it in the
/// error message. The encoding of the data is driver-defined.
+/// Drivers may provide multiple error details.
///
-/// This can be called immediately after any API call that returns an
+/// This can be used immediately after any API call that returns an
/// error. Additionally, if an ArrowArrayStream returned from an
/// AdbcConnection or an AdbcStatement returns an error, this can be
/// immediately called from the associated AdbcConnection or
/// AdbcStatement to get further error details (if available). Making
/// other API calls with that connection or statement may clear this
/// error value.
///
-/// Drivers may provide multiple error details. Each call to
-/// GetOptionBytes will return the next error detail. The driver
-/// should return ADBC_STATUS_NOT_FOUND if there are no (more) error
-/// details.
+/// To use, call GetOptionInt with this option to get the number of
+/// available details. Then, call GetOption with the option key
+/// ADBC_OPTION_ERROR_DETAILS_PREFIX + (zero-indexed index) to get the
+/// name of the error detail (for example, drivers that use gRPC
+/// underneath may provide the name of the gRPC trailer corresponding
+/// to the error detail). GetOptionBytes with that option name will
+/// retrieve the value of the error detail (for example, a serialized
+/// Any-wrapped Protobuf).
///
-/// The type is uint8_t*.
+/// \since ADBC API revision 1.1.0
+/// \addtogroup adbc-1.1.0
+#define ADBC_OPTION_ERROR_DETAILS "adbc.error_details"
+
+/// \brief Canonical option name for error details.
///
+/// \see ADBC_OPTION_ERROR_DETAILS
/// \since ADBC API revision 1.1.0
/// \addtogroup adbc-1.1.0
-#define ADBC_OPTION_ERROR_DETAILS "error_details"
+#define ADBC_OPTION_ERROR_DETAILS_PREFIX "adbc.error_details."
Review Comment:
should the corresponding constants get added to `adbc.go` ?
##########
go/adbc/driver/flightsql/flightsql_adbc.go:
##########
@@ -1154,6 +1150,8 @@ func (c *cnxn) GetInfo(ctx context.Context, infoCodes
[]adbc.InfoCode) (array.Re
infoNameBldr.Append(uint32(adbc.InfoVendorVersion))
case
flightsql.SqlInfoFlightSqlServerArrowVersion:
infoNameBldr.Append(uint32(adbc.InfoVendorArrowVersion))
+ default:
+ continue
Review Comment:
was this found by a test? if not can we add a test that would hit this?
##########
go/adbc/pkg/_tmpl/driver.go.tmpl:
##########
@@ -115,6 +198,44 @@ func getFromHandle[T any](ptr unsafe.Pointer) *T {
return cgo.Handle((uintptr)(*hptr)).Value().(*T)
}
+func exportStringOption(val string, out *C.char, length *C.size_t)
C.AdbcStatusCode {
+ lenWithTerminator := C.size_t(len(val) + 1)
+ if lenWithTerminator <= *length {
+ // TODO: doesn't this reallocate the string? can we avoid this?
+ C.strncpy(out, (*C.char)(C.CString(val)), lenWithTerminator)
+ }
+ *length = lenWithTerminator
+ return C.ADBC_STATUS_OK
+}
+
+func exportBytesOption(val []byte, out *C.uint8_t, length *C.size_t)
C.AdbcStatusCode {
+ if C.size_t(len(val)) <= *length {
+ sink := fromCArr[byte]((*byte)(out), int(*length))
+ copy(sink, val)
+ }
Review Comment:
should we throw some error if the buffer they provided isn't large enough?
or should it just truncate (in which case you don't need the length check since
`copy` will already truncate if the source is larger than the destination)
##########
go/adbc/pkg/_tmpl/driver.go.tmpl:
##########
@@ -426,6 +911,46 @@ func toCdataArray(ptr *C.struct_ArrowArray)
*cdata.CArrowArray {
return (*cdata.CArrowArray)(unsafe.Pointer(ptr))
}
+//export {{.Prefix}}ConnectionCancel
+func {{.Prefix}}ConnectionCancel(cnxn *C.struct_AdbcConnection, err
*C.struct_AdbcError) (code C.AdbcStatusCode) {
+ defer func() {
+ if e := recover(); e != nil {
+ code = poison(err, "AdbcConnectionCancel", e)
+ }
+ }()
+ conn := checkConnInit(cnxn, err, "AdbcConnectionCancel")
+ if conn == nil {
+ return C.ADBC_STATUS_INVALID_STATE
+ }
+
+ conn.cancelContext()
+ return C.ADBC_STATUS_OK
+}
+
+func toStrPtr(in *C.cchar_t) *string {
+ if in == nil {
+ return nil
+ }
+
+ out := C.GoString((*C.char)(in))
+ return &out
+}
+
+func toStrSlice(in **C.cchar_t) []string {
+ if in == nil {
+ return nil
+ }
+
+ sz := unsafe.Sizeof(*in)
+
+ out := make([]string, 0, 1)
+ for *in != nil {
+ out = append(out, C.GoString(*in))
+ in = (**C.cchar_t)(unsafe.Add(unsafe.Pointer(in), sz))
Review Comment:
the only thing that sucks here is that we're duplicating all the strings :(
##########
go/adbc/pkg/_tmpl/driver.go.tmpl:
##########
@@ -895,13 +1685,18 @@ func {{.Prefix}}StatementExecutePartitions(stmt
*C.struct_AdbcStatement, schema
//export {{.Prefix}}DriverInit
func {{.Prefix}}DriverInit(version C.int, rawDriver *C.void, err
*C.struct_AdbcError) C.AdbcStatusCode {
- if version != C.ADBC_VERSION_1_0_0 {
- setErr(err, "Only version %d supported, got %d",
int(C.ADBC_VERSION_1_0_0), int(version))
+ driver := (*C.struct_AdbcDriver)(unsafe.Pointer(rawDriver))
+
+ switch version {
+ case C.ADBC_VERSION_1_0_0:
+ C.memset(unsafe.Pointer(driver), 0, C.ADBC_DRIVER_1_0_0_SIZE)
Review Comment:
you can use `memory.Set` for this, the Go Arrow lib already has an optimized
memset (it'll use SIMD for the memset if available even!) it also avoids the
CGO call here since it's native go (or go asm) and therefore will be faster
##########
go/adbc/pkg/_tmpl/driver.go.tmpl:
##########
@@ -85,6 +87,87 @@ func errToAdbcErr(adbcerr *C.struct_AdbcError, err error)
adbc.Status {
return adbc.StatusUnknown
}
+// errorDetails handles the standard error details feature in ADBC.
+type errorDetails struct {
+ errorDetails []adbc.ErrorDetail
+}
+
Review Comment:
this feels like something which might be better to extract into some
internal utility module and called here, rather than having it in the template
and get duplicated in every driver code, yes/no?
##########
go/adbc/pkg/_tmpl/driver.go.tmpl:
##########
@@ -426,6 +911,46 @@ func toCdataArray(ptr *C.struct_ArrowArray)
*cdata.CArrowArray {
return (*cdata.CArrowArray)(unsafe.Pointer(ptr))
}
+//export {{.Prefix}}ConnectionCancel
+func {{.Prefix}}ConnectionCancel(cnxn *C.struct_AdbcConnection, err
*C.struct_AdbcError) (code C.AdbcStatusCode) {
+ defer func() {
+ if e := recover(); e != nil {
+ code = poison(err, "AdbcConnectionCancel", e)
+ }
+ }()
+ conn := checkConnInit(cnxn, err, "AdbcConnectionCancel")
+ if conn == nil {
+ return C.ADBC_STATUS_INVALID_STATE
+ }
+
+ conn.cancelContext()
+ return C.ADBC_STATUS_OK
+}
+
+func toStrPtr(in *C.cchar_t) *string {
+ if in == nil {
+ return nil
+ }
+
+ out := C.GoString((*C.char)(in))
+ return &out
Review Comment:
if we bump to go1.20 we could avoid the string copy potentially by doing
`unsafe.String((*byte)(unsafe.Pointer(in), int(C.strlen(in)))` to get the
string, if you think it matters (not sure if it's worthwhile or not)
##########
go/adbc/pkg/_tmpl/driver.go.tmpl:
##########
@@ -115,6 +198,44 @@ func getFromHandle[T any](ptr unsafe.Pointer) *T {
return cgo.Handle((uintptr)(*hptr)).Value().(*T)
}
+func exportStringOption(val string, out *C.char, length *C.size_t)
C.AdbcStatusCode {
+ lenWithTerminator := C.size_t(len(val) + 1)
+ if lenWithTerminator <= *length {
+ // TODO: doesn't this reallocate the string? can we avoid this?
+ C.strncpy(out, (*C.char)(C.CString(val)), lenWithTerminator)
Review Comment:
not only does `C.CString` reallocate the string, you have a memory leak here
because you aren't calling free on the result.
Why not just grab the raw bytes of the string and copy those to out and then
add the null terminator manually? That would avoid the allocation.
If we bump to using go1.20 you can use https://pkg.go.dev/unsafe#StringData
to get the raw bytes, alternatively we can do it the less safe way
```go
cout := fromCArr[byte](out, *length)
copy(out, val)
*(*byte)(unsafe.Add(unsafe.Pointer(out), len(val))) = 0
```
Or something like that....?
##########
go/adbc/pkg/_tmpl/driver.go.tmpl:
##########
@@ -71,7 +73,7 @@ func setErr(err *C.struct_AdbcError, format string, vals
...interface{}) {
}
func errToAdbcErr(adbcerr *C.struct_AdbcError, err error) adbc.Status {
- if adbcerr == nil || err == nil {
Review Comment:
we don't need to verify that the adbc error isn't null anymore? we can
assume that it's always a valid pointer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
