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(go/adbc/driver/flightsql): Improvements for flightSql error details #2185

Closed
wants to merge 6 commits into from
Closed

feat(go/adbc/driver/flightsql): Improvements for flightSql error details #2185

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c0f714b
Comment fixes
rshura Sep 24, 2024
d767fe6
Helper function to get adbc.Error from error
rshura Sep 24, 2024
713a93e
Use constant for grpc-status-details-bin
rshura Sep 24, 2024
6dfc6a9
Skip adding details whose values start with Grpc-
rshura Sep 24, 2024
531e148
Try fixing tests.
rshura Sep 24, 2024
8333e89
Use errors.Is instead of a new helper function.
rshura Sep 24, 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
4 changes: 2 additions & 2 deletions go/adbc/adbc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
23 changes: 19 additions & 4 deletions go/adbc/driver/flightsql/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package flightsql

import (
"context"
"errors"
"fmt"
"strings"

Expand All @@ -30,13 +31,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 := errors.Is(err, adbc.Error{}); ok {
return err
}

Expand Down Expand Up @@ -84,7 +87,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
Expand All @@ -97,7 +106,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"):
Expand All @@ -107,6 +116,9 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con
}
default:
for _, value := range values {
if strings.HasPrefix(value, "Grpc-") {
continue
}
Comment on lines +119 to +121
Copy link
Member

Choose a reason for hiding this comment

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

why skip things with Grpc- prefix?

Copy link
Author

Choose a reason for hiding this comment

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

I don't know why and how, but I was getting TextErrorDetails with key trailer and value Grpc-Status and Grpc-Message and Grpc-Status-Details-Bin. I don't think those values can be useful.

details = append(details, &adbc.TextErrorDetail{Name: key, Detail: value})
}
}
Expand All @@ -116,7 +128,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"):
Expand All @@ -126,6 +138,9 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con
}
default:
for _, value := range values {
if strings.HasPrefix(value, "Grpc-") {
continue
}
Comment on lines +141 to +143
Copy link
Member

Choose a reason for hiding this comment

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

same question as above

details = append(details, &adbc.TextErrorDetail{Name: key, Detail: value})
}
}
Expand Down
5 changes: 1 addition & 4 deletions python/adbc_driver_flightsql/tests/test_errors.py
View file
Open in desktop
Copy link
Member

Choose a reason for hiding this comment

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

Hmm. Isn't this strictly worse now? Because without any, you have to know the actual Protobuf type in advance.

Copy link
Author

Choose a reason for hiding this comment

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

Yes it appears so. I guess we can scrap this PR if there's nothing useful.

Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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"}

Expand Down
5 changes: 1 addition & 4 deletions python/adbc_driver_flightsql/tests/test_incremental.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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"}

Expand Down
Loading