[GitHub] [arrow-adbc] zeroshade commented on a diff in pull request #893: feat(go/adbc): add FFI support for 1.1.0 features

["zeroshade (via GitHub)" <gi...@apache.org>]

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: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org