tustvold commented on code in PR #3594:
URL: https://github.com/apache/arrow-rs/pull/3594#discussion_r1085474558
##########
arrow-flight/src/decode.rs:
##########
@@ -219,6 +214,11 @@ impl FlightDataDecoder {
}
}
+ /// Returns the current schema for this stream
+ pub fn schema(&self) -> Option<SchemaRef> {
+ self.state.as_ref().map(|state| state.schema.clone())
Review Comment:
```suggestion
pub fn schema(&self) -> Option<&SchemaRef> {
self.state.as_ref()
```
##########
arrow-flight/src/encode.rs:
##########
@@ -250,9 +280,8 @@ impl Stream for FlightDataEncoder {
None => {
// inner is done
self.done = true;
- // queue must also be empty so we are done
- assert!(self.queue.is_empty());
- return Poll::Ready(None);
+ // queue might still have a message (if the stream
Review Comment:
I'm confused by this change, we have a mutable reference to `Self` so
`queue` cannot be extended whilst in scope, and we check a few lines above that
`self.queue.pop_front().is_empty()`
##########
arrow-flight/src/decode.rs:
##########
@@ -101,13 +97,12 @@ impl FlightRecordBatchStream {
{
Self {
inner: FlightDataDecoder::new(inner),
- got_schema: false,
}
}
- /// Has a message defining the schema been received yet?
- pub fn got_schema(&self) -> bool {
- self.got_schema
+ /// Return schema for the stream, if it has been received
+ pub fn schema(&self) -> Option<SchemaRef> {
Review Comment:
```suggestion
/// Return schema for the stream, if it has been received
pub fn schema(&self) -> Option<&SchemaRef> {
```
Returning owned types is a code smell in most cases, as it breaks borrow
propagation
##########
arrow-flight/src/decode.rs:
##########
@@ -101,13 +97,12 @@ impl FlightRecordBatchStream {
{
Self {
inner: FlightDataDecoder::new(inner),
- got_schema: false,
}
}
- /// Has a message defining the schema been received yet?
- pub fn got_schema(&self) -> bool {
Review Comment:
Is it worth keeping this method to avoid this being a breaking change?
##########
arrow-flight/src/encode.rs:
##########
@@ -189,26 +215,30 @@ impl FlightDataEncoder {
}
}
+ /// Encodes schema as a [`FlightData`] in self.queue.
+ /// Updates `self.schema` and returns the new schema
+ fn encode_schema(&mut self, schema: SchemaRef) -> SchemaRef {
Review Comment:
```suggestion
/// Updates `self.schema` and returns the new schema
fn encode_schema(&mut self, schema: &Schema) {
```
Might be a more obvious signature imo
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
