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.

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

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

Closed
wants to merge 6 commits into from

Conversation

rshura
Copy link

@rshura rshura commented Sep 24, 2024

See #2179

@github-actions github-actions bot added this to the ADBC Libraries 15 milestone Sep 24, 2024
@zeroshade zeroshade changed the title Improvements for flightSql error details feat(go/adbc/driver/flightsql): Improvements for flightSql error details Sep 24, 2024
@rshura rshura changed the title feat(go/adbc/driver/flightsql): Improvements for flightSql error details GH-2185: [Go][FlightSQL] Improvements for flightSql error details Sep 24, 2024
@rshura rshura changed the title GH-2185: [Go][FlightSQL] Improvements for flightSql error details feat(go/adbc/driver/flightsql): Improvements for flightSql error details Sep 24, 2024
go/adbc/adbc.go Outdated Show resolved Hide resolved
go/adbc/driver/flightsql/utils.go Outdated Show resolved Hide resolved
Comment on lines +118 to +120
if strings.HasPrefix(value, "Grpc-") {
continue
}
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.

Comment on lines +140 to +142
if strings.HasPrefix(value, "Grpc-") {
continue
}
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

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.

@rshura
Copy link
Author

rshura commented Sep 25, 2024

Closing as it seems to not improve anything.

@rshura rshura closed this Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants