You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "rshura (via GitHub)" <gi...@apache.org> on 2024/09/24 16:13:25 UTC
[PR] Improvements for flightSql error details [arrow-adbc]
rshura opened a new pull request, #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185
See https://github.com/apache/arrow-adbc/issues/2179
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774230953
##########
python/adbc_driver_flightsql/tests/test_errors.py:
##########
Review Comment:
Hmm. Isn't this strictly worse now? Because without any, you have to know the actual Protobuf type in advance.
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774161542
##########
go/adbc/adbc.go:
##########
@@ -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
+}
Review Comment:
Dropped the new function.
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774133712
##########
go/adbc/driver/flightsql/utils.go:
##########
@@ -107,6 +115,9 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con
}
default:
for _, value := range values {
+ if strings.HasPrefix(value, "Grpc-") {
+ continue
+ }
Review Comment:
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.
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774234211
##########
python/adbc_driver_flightsql/tests/test_errors.py:
##########
Review Comment:
Yes it appears so. I guess we can scrap this PR if there's nothing useful.
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774122488
##########
go/adbc/driver/flightsql/utils.go:
##########
@@ -126,6 +137,9 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con
}
default:
for _, value := range values {
+ if strings.HasPrefix(value, "Grpc-") {
+ continue
+ }
Review Comment:
same question as above
##########
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 {
Review Comment:
`if errors.Is(err, adbc.Error{}) {`
##########
go/adbc/adbc.go:
##########
@@ -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
+}
Review Comment:
I don't find this particular function very useful. Consumers should just use `errors.As` and `errors.Is` themselves rather than needing to use this function directly
##########
go/adbc/driver/flightsql/utils.go:
##########
@@ -107,6 +115,9 @@ func adbcFromFlightStatusWithDetails(err error, header, trailer metadata.MD, con
}
default:
for _, value := range values {
+ if strings.HasPrefix(value, "Grpc-") {
+ continue
+ }
Review Comment:
why skip things with `Grpc-` prefix?
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#issuecomment-2372766282
Closing as it seems to not improve anything.
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774134255
##########
go/adbc/adbc.go:
##########
@@ -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
+}
Review Comment:
Fine, `error.Is` it is
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura closed pull request #2185: feat(go/adbc/driver/flightsql): Improvements for flightSql error details
URL: https://github.com/apache/arrow-adbc/pull/2185
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774132636
##########
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 {
Review Comment:
agreed.
--
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
Re: [PR] feat(go/adbc/driver/flightsql): Improvements for flightSql error details [arrow-adbc]
Posted by "rshura (via GitHub)" <gi...@apache.org>.
rshura commented on code in PR #2185:
URL: https://github.com/apache/arrow-adbc/pull/2185#discussion_r1774161835
##########
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 {
Review Comment:
Using errors.Is now.
--
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