Open matthewmturner opened 4 months ago
@matthewmturner I just want to clarify something, the response from DoPut
is a PutResult
message that contains a single bytes
field named app_metadata
. In the case of the DoPut
for DoPutPreparedStatementResult
we were deserializing the app_metadata
directly into a DoPutPreparedStatementResult
, which contains the handle as a bytes
member. Are you expecting the app_metadata
of PutResult
to be a an Any
protobuf in order to retrieve the DoPutPreparedStatementResult
? Or are you expecting the handle
member to be an Any
?
@lidavidm Since previously DoPutPreparedStatement
didn't return anything until we added the DoPutPreparedStatementResult
for updating the handle, there wasn't a convention for what should be returned. Looking at the results from the other messages I see the following general convention:
DoAction
(BeginSavepointResult, BeginTransactionResult, CancelQueryResult) are assumed to be serialized in an Any
DoPut
such as DoPutUpdateResult
, are not assumed to be serialized as an Any
inside of the app_metadata
field of PutResult
.That said, it does appear that the Go implementation is outnumbered here as the C++ implementation does assume an Any
wrapper around the DoPutPreparedStatementResult
, but it just seems odd to me as it breaks the existing convention that we had, leaving it to be kind of arbitrary and not documented anywhere.
@zeroshade appreciate the quick reply.
As for my expectations, im just expecting there to be consistency across the different language implementations. I thought that the spec would define how this is supposed to look on the wire.
Im personally more aligned with the Any
wrapper around the DoPutPreparedStatementResult
though which I believe would make it easier to deserialize and add logic on the client side if needed.
Also FWIW on the rust side it looks like everything within do_put
is wrapped in Any
Can we get Rust in compliance? This is rather concerning. If we need to expand the integration tests then let's do so. Or we should tackle the old issue about using a pure gRPC+Protobuf client without Arrow libraries to do some conformance testing.
The idea of Flight SQL is that we shouldn't be adding extra client logic. So I would be against the Any wrapper. (It's also probably redundant on the request side when used in DoAction but that ship has sailed.)
+this is more reason to disentangle Flight-as-an-Arrow-transport from Flight-as-a-forced-service-definition since being able to define a proper gRPC service for Flight SQL would make these mistakes harder/impossible and would make testing, implementation, and telemetry much easier
@lidavidm thank you for the feedback. I will raise the issue on the Rust side.
@lidavidm Also, can you clarify what compliance means in this case? I would have thought compliance refers to following spec but it seems were discussing convention here. And to confirm does this mean the C++ implementation will also be updated to no longer use Any
?
Right, the spec is insufficient here because of Flight RPC (if this were a gRPC service definition there would be no issue). So we should fix the spec too and add better conformance testing.
I would like the spec here to follow the existing convention and not use Any, as the result type should be a fixed quantity in this case. (Really, nothing should have used Any in the first place, but it's too late for that.) I'm open to being convinced otherwise, though
@lidavidm makes sense and definitely agree on the conformance testing. my only concern is that there may be some language specific conventions that could be conflicting. separate from the issues in these repos, and in particular if there will be a spec change, this is probably worth an email thread to gather more opinions. i can work on that tomorrow.
The rationale behind the Any
wrapper is that it is consistent with other command-specific responses in the arrow-rs
implementation. Note the as_any
method calls on each of the specific command types here:
Is this behavior inconsistent with the other arrow clients?
I think it would be confusing to handle this result type differently from the others.
@lidavidm makes sense and definitely agree on the conformance testing. my only concern is that there may be some language specific conventions that could be conflicting. separate from the issues in these repos, and in particular if there will be a spec change, this is probably worth an email thread to gather more opinions. i can work on that tomorrow.
Yes, if there are language convention differences in how the app_metadata
is encoded across all of the PutResult
message types then that would suggest a larger problem that needs to be addressed. That would be surprising considering that no divergent behavior has been reported until now.
I don't think there's been enough independent implementations, really, and I don't think there's too much cross-testing (e.g. ADBC gets tested against the Go SQLite sample and the OSS edition of Dremio, but we don't have a test for InfluxDB, IIRC last I looked Flight SQL was only in the cloud product?)
@lidavidm @erratic-pattern thank you both for the feedback.
I reviewed some more and agree with the proposal that the Rust implementation should be updated to not use Any
. Let me know if you think I am mistaken in my analysis.
Sounds good to me, thank you.
@zeroshade is there any work going on to both clarify the spec and test it more thoroughly?
Not currently on my end. But i can add it to my list of stuff and make sure that we get around to it
I have a PR for arrow-rs https://github.com/apache/arrow-rs/pull/5817 I have not tested yet how it behaves with other client/server implementations.
Is there anything left to do here? I think with closed all of the implementations should be consistent now. That being said, having an open issue for cross-language testing would be good. Maybe that should be raised as a separate issue?
A separate issue for cross-language testing would be good
Describe the bug, including details regarding any error messages, version, and platform.
We just upgraded our Go FlightSQL client to point to master so that we can use the recently added stateless prepared statements (https://github.com/apache/arrow/pull/41428 and https://github.com/apache/arrow/pull/40311). However, we have had issues while trying to use this when getting data from our Rust FlightSQL server. Specifically, we have been unable to deserialize the
DoPutPreparedStatementResult
that the server is returning.We realized that the Rust implementation is returning
Any
protobuf messages (which we also verified in the protobuf messages being sent across the wire) but the go FlightSQL client is not deserializing usinganypb
to decode this message. Given that the prepared statement handle is meant to represent an opaque handle it seemed appropriate for it to useAny
so I believe the Go side should be updated to use this - but let me know if I am mistaken.I am also raising this to the Rust community to confirm that returning
Any
messages was intentional.Component(s)
Go