From c0f714b7ea8c3efbc7d156e8cfef08b27d709915 Mon Sep 17 00:00:00 2001 From: Alex Roitman Date: Tue, 24 Sep 2024 11:48:03 -0400 Subject: [PATCH 1/6] Comment fixes --- go/adbc/adbc.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/go/adbc/adbc.go b/go/adbc/adbc.go index 636320fa31..6920195b84 100644 --- a/go/adbc/adbc.go +++ b/go/adbc/adbc.go @@ -85,7 +85,7 @@ func (d *ProtobufErrorDetail) Serialize() ([]byte, error) { return proto.Marshal(any) } -// ProtobufErrorDetail is an ErrorDetail backed by a human-readable string. +// TextErrorDetail is an ErrorDetail backed by a human-readable string. type TextErrorDetail struct { Name string Detail string @@ -100,7 +100,7 @@ func (d *TextErrorDetail) Serialize() ([]byte, error) { return []byte(d.Detail), nil } -// ProtobufErrorDetail is an ErrorDetail backed by a binary payload. +// BinaryErrorDetail is an ErrorDetail backed by a binary payload. type BinaryErrorDetail struct { Name string Detail []byte From d767fe6303b5fb87a6dea541ff51a36c8e392901 Mon Sep 17 00:00:00 2001 From: Alex Roitman Date: Tue, 24 Sep 2024 11:49:40 -0400 Subject: [PATCH 2/6] Helper function to get adbc.Error from error --- go/adbc/adbc.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/go/adbc/adbc.go b/go/adbc/adbc.go index 6920195b84..f19f1714c5 100644 --- a/go/adbc/adbc.go +++ b/go/adbc/adbc.go @@ -38,6 +38,7 @@ package adbc import ( "context" + "errors" "fmt" "github.com/apache/arrow/go/v18/arrow" @@ -130,6 +131,16 @@ type Error struct { Details []ErrorDetail } +// AdbcError helper function returns an adbc.Error representation +// of a given error +func AdbcError(err error) (*Error, bool) { + var adbcErr Error + if errors.As(err, &adbcErr) { + return &adbcErr, true + } + return nil, false +} + func (e Error) Error() string { // Don't include a NUL in the string since C Data Interface uses char* (and // don't include the extra cruft if not needed in the first place) From 713a93e90f367206ed86d51c5cfc8396f34dad71 Mon Sep 17 00:00:00 2001 From: Alex Roitman Date: Tue, 24 Sep 2024 12:08:33 -0400 Subject: [PATCH 3/6] Use constant for grpc-status-details-bin Use &adbc.ProtobufErrorDetail if the detail can be decoded into proto --- go/adbc/driver/flightsql/utils.go | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/go/adbc/driver/flightsql/utils.go b/go/adbc/driver/flightsql/utils.go index acd3ca2dea..9064d91269 100644 --- a/go/adbc/driver/flightsql/utils.go +++ b/go/adbc/driver/flightsql/utils.go @@ -30,13 +30,15 @@ import ( "google.golang.org/protobuf/types/known/anypb" ) +const grpcStatusDetailsBin = "grpc-status-details-bin" + func adbcFromFlightStatus(err error, context string, args ...any) error { var header, trailer metadata.MD return adbcFromFlightStatusWithDetails(err, header, trailer, context, args...) } func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, context string, args ...any) error { - if _, ok := err.(adbc.Error); ok { + if _, ok := adbc.AdbcError(err); ok { return err } @@ -84,7 +86,13 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con details := []adbc.ErrorDetail{} for _, detail := range grpcStatus.Proto().Details { - details = append(details, &anyErrorDetail{name: "grpc-status-details-bin", message: detail}) + if detailProto, err := detail.UnmarshalNew(); err != nil { + // if we could decode into proto, use ProtobufErrorDetail + details = append(details, &adbc.ProtobufErrorDetail{Name: grpcStatusDetailsBin, Message: detailProto}) + } else { + // proceed with any, maybe the user knows how to deserialize? + details = append(details, &anyErrorDetail{name: grpcStatusDetailsBin, message: detail}) + } } // XXX(https://github.com/grpc/grpc-go/issues/5485): don't count on @@ -97,7 +105,7 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con case key == "content-type": // Not useful info continue - case key == "grpc-status-details-bin": + case key == grpcStatusDetailsBin: // gRPC library parses this above via grpcStatus.Proto() continue case strings.HasSuffix(key, "-bin"): @@ -116,7 +124,7 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con case key == "content-type": // Not useful info continue - case key == "grpc-status-details-bin": + case key == grpcStatusDetailsBin: // gRPC library parses this above via grpcStatus.Proto() continue case strings.HasSuffix(key, "-bin"): From 6dfc6a9cc8e334d43aac4554f66d9c39c731f399 Mon Sep 17 00:00:00 2001 From: Alex Roitman Date: Tue, 24 Sep 2024 12:09:25 -0400 Subject: [PATCH 4/6] Skip adding details whose values start with Grpc- --- go/adbc/driver/flightsql/utils.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/go/adbc/driver/flightsql/utils.go b/go/adbc/driver/flightsql/utils.go index 9064d91269..10752f1860 100644 --- a/go/adbc/driver/flightsql/utils.go +++ b/go/adbc/driver/flightsql/utils.go @@ -115,6 +115,9 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con } default: for _, value := range values { + if strings.HasPrefix(value, "Grpc-") { + continue + } details = append(details, &adbc.TextErrorDetail{Name: key, Detail: value}) } } @@ -134,6 +137,9 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con } default: for _, value := range values { + if strings.HasPrefix(value, "Grpc-") { + continue + } details = append(details, &adbc.TextErrorDetail{Name: key, Detail: value}) } } From 531e148522419dc2766656b9c7100dc7ac543d83 Mon Sep 17 00:00:00 2001 From: Alex Roitman Date: Tue, 24 Sep 2024 18:17:57 -0400 Subject: [PATCH 5/6] Try fixing tests. --- python/adbc_driver_flightsql/tests/test_errors.py | 5 +---- python/adbc_driver_flightsql/tests/test_incremental.py | 5 +---- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/python/adbc_driver_flightsql/tests/test_errors.py b/python/adbc_driver_flightsql/tests/test_errors.py index d572589aee..e90d79f2ba 100644 --- a/python/adbc_driver_flightsql/tests/test_errors.py +++ b/python/adbc_driver_flightsql/tests/test_errors.py @@ -21,7 +21,6 @@ import threading import time -import google.protobuf.any_pb2 as any_pb2 import google.protobuf.wrappers_pb2 as wrappers_pb2 import pytest @@ -30,10 +29,8 @@ def assert_detail(e): # Check that the expected error details are present found = set() for _, detail in e.details: - anyproto = any_pb2.Any() - anyproto.ParseFromString(detail) string = wrappers_pb2.StringValue() - anyproto.Unpack(string) + string.ParseFromString(detail) found.add(string.value) assert found == {"detail1", "detail2"} diff --git a/python/adbc_driver_flightsql/tests/test_incremental.py b/python/adbc_driver_flightsql/tests/test_incremental.py index 285c18a831..2088da8a00 100644 --- a/python/adbc_driver_flightsql/tests/test_incremental.py +++ b/python/adbc_driver_flightsql/tests/test_incremental.py @@ -18,7 +18,6 @@ import re import threading -import google.protobuf.any_pb2 as any_pb2 import google.protobuf.wrappers_pb2 as wrappers_pb2 import pyarrow import pyarrow.flight @@ -46,10 +45,8 @@ def test_incremental_error(test_dbapi) -> None: found = set() for _, detail in exc_info.value.details: - anyproto = any_pb2.Any() - anyproto.ParseFromString(detail) string = wrappers_pb2.StringValue() - anyproto.Unpack(string) + string.ParseFromString(detail) found.add(string.value) assert found == {"detail1", "detail2"} From 8333e89ada3f2625092258a61c7bef143cd36d85 Mon Sep 17 00:00:00 2001 From: Alex Roitman Date: Tue, 24 Sep 2024 18:21:52 -0400 Subject: [PATCH 6/6] Use errors.Is instead of a new helper function. --- go/adbc/adbc.go | 11 ----------- go/adbc/driver/flightsql/utils.go | 3 ++- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/go/adbc/adbc.go b/go/adbc/adbc.go index f19f1714c5..6920195b84 100644 --- a/go/adbc/adbc.go +++ b/go/adbc/adbc.go @@ -38,7 +38,6 @@ package adbc import ( "context" - "errors" "fmt" "github.com/apache/arrow/go/v18/arrow" @@ -131,16 +130,6 @@ type Error struct { Details []ErrorDetail } -// AdbcError helper function returns an adbc.Error representation -// of a given error -func AdbcError(err error) (*Error, bool) { - var adbcErr Error - if errors.As(err, &adbcErr) { - return &adbcErr, true - } - return nil, false -} - func (e Error) Error() string { // Don't include a NUL in the string since C Data Interface uses char* (and // don't include the extra cruft if not needed in the first place) diff --git a/go/adbc/driver/flightsql/utils.go b/go/adbc/driver/flightsql/utils.go index 10752f1860..52824c8d26 100644 --- a/go/adbc/driver/flightsql/utils.go +++ b/go/adbc/driver/flightsql/utils.go @@ -19,6 +19,7 @@ package flightsql import ( "context" + "errors" "fmt" "strings" @@ -38,7 +39,7 @@ func adbcFromFlightStatus(err error, context string, args ...any) error { } func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, context string, args ...any) error { - if _, ok := adbc.AdbcError(err); ok { + if ok := errors.Is(err, adbc.Error{}); ok { return err }