feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

Weijun-H · 2023-04-19T22:31:46Z

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold · 2023-04-20T19:39:33Z

arrow-flight/src/encode.rs

        )
    }
 }

 /// Stream that encodes a stream of record batches to flight data.
 ///
 /// See [`FlightDataEncoderBuilder`] for details and example.
+#[warn(dead_code)]


Why this change?

tustvold · 2023-04-20T19:42:00Z

arrow-flight/src/encode.rs

+        for mut data in datas {
+            // The first message is the schema message need to set the descriptor
+            if is_first {
+                data.flight_descriptor = self.descriptor.clone();


I think the ticket calls for sending a separate descriptor message, not overriding the first message

Weijun-H · 2023-04-20T21:05:05Z

Error happened, but not by my pr

Run cargo fmt --all -- --check
Diff in /__w/arrow-rs/arrow-rs/arrow-string/src/like.rs at line 1244:
             "ﬀkoß",
             "😃sadlksFFkoSSsh😃klF", // Original was case insensitive "😃sadlksffkosSsh😃klF"
             "😱slgFFkoSSsh😃klF",    // Original was case insensitive "😱slgffkosSsh😃klF"
-            "FFkoSS",                    // "FFKoSS"
+            "FFkoSS",                // "FFKoSS"
         ],
         "FFkoSS",
         contains_utf[8](https://github.com/apache/arrow-rs/actions/runs/4758784682/jobs/8457275692?pr=4101#step:6:9)_scalar,

alamb

Thank you @Weijun-H

I think this PR needs:

A test of this feature (as I don't think it is working as expected, but I may be mistaken. A test will show us one way or the other). Perhaps in
https://github.com/apache/arrow-rs/blob/master/arrow-flight/tests/encode_decode.rs
Some documentation describing when the flight descriptor is sent

arrow-flight/src/encode.rs

alamb · 2023-04-22T11:33:05Z

arrow-flight/src/encode.rs

+        );
+        self.queue_message(descriptor_flight_data);
+        // remember descriptor
+        self.descriptor = Some(descriptor.clone());


Why is the descriptor remembered?

alamb · 2023-04-22T11:33:23Z

arrow-flight/src/encode.rs

@@ -176,6 +189,8 @@ pub struct FlightDataEncoder {
    queue: VecDeque<FlightData>,
    /// Is this stream done (inner is empty or errored)
    done: bool,
+    /// descriptor, set after the first batch


What does "set after the first batch" mean?

I guess I expect this to be something more like "cleared after the first FlightData message"

alamb · 2023-04-22T11:39:15Z

arrow-flight/src/encode.rs

        };

        // If schema is known up front, enqueue it immediately
        if let Some(schema) = schema {
            encoder.encode_schema(&schema);
        }
+
+        // If descriptor is known up front, enqueue it immediately
+        if let Some(descriptor) = descriptor {


As I understand the intent, the idea is that the descriptor is set on the first message that is sent this code will potentially send a separate message after the schema message, if the schema is known up front.

Perhaps you could check self.descriptor in enqueue_messages and attach the descriptor there

alamb

THanks @Weijun-H -- this is looking close

alamb · 2023-04-24T20:05:30Z

arrow-flight/src/encode.rs

        };

        // If schema is known up front, enqueue it immediately
        if let Some(schema) = schema {
            encoder.encode_schema(&schema);
        }
+
+        encoder.descriptor = descriptor;


I think this should be set before calling encoder.encode_schema() so it is included on the first Schema message, if provided

alamb · 2023-04-24T20:06:56Z

arrow-flight/src/encode.rs

    fn queue_message(&mut self, data: FlightData) {
+        if let Some(descriptor) = &self.descriptor {
+            let mut data = data;
+            data.flight_descriptor = Some(descriptor.clone());
+            self.descriptor = None;
+            self.queue.push_back(data);
+            return;
+        }
        self.queue.push_back(data);
    }


FWIW I think you can write this using take() a bit more concisely like:

Suggested change

fn queue_message(&mut self, data: FlightData) {

if let Some(descriptor) = &self.descriptor {

let mut data = data;

data.flight_descriptor = Some(descriptor.clone());

self.descriptor = None;

self.queue.push_back(data);

return;

}

self.queue.push_back(data);

}

fn queue_message(&mut self, mut data: FlightData) {

if let Some(descriptor) = self.descriptor.take() {

data.flight_descriptor = Some(descriptor.clone());

}

self.queue.push_back(data);

}

alamb · 2023-04-24T20:08:18Z

arrow-flight/tests/encode_decode.rs

+    let mut encoder = encoder.build(stream);
+
+    // First batch should be the schema
+    encoder.next().await.unwrap().unwrap();


My understanding of https://arrow.apache.org/docs/format/Flight.html#uploading-data was that the first FlightData should have the descriptor, not the second 🤔

Yes, you are right. I completely misunderstood it.

alamb

Look good -- thank you @Weijun-H

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Apr 19, 2023

tustvold reviewed Apr 20, 2023

View reviewed changes

Weijun-H added 2 commits April 21, 2023 10:42

feat: set FlightDescriptor on FlightDataEncoderBuilder

d66167b

send a separate descriptor message when the descriptor is provided

c48c00f

Weijun-H force-pushed the set-FlightDescriptor-on-FlightDataEncoderBuilder branch from 01d384b to c48c00f Compare April 21, 2023 08:42

alamb reviewed Apr 22, 2023

View reviewed changes

alamb reviewed Apr 24, 2023

View reviewed changes

include the flight descriptor in the first FlightData

99af00d

Weijun-H force-pushed the set-FlightDescriptor-on-FlightDataEncoderBuilder branch from dd3c32e to 99af00d Compare April 24, 2023 20:20

alamb approved these changes Apr 25, 2023

View reviewed changes

alamb merged commit 0059049 into apache:master Apr 25, 2023

tustvold mentioned this pull request May 5, 2023

Enable setting FlightDescriptor on FlightDataEncoderBuilder #3855

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

Weijun-H commented Apr 19, 2023

tustvold Apr 20, 2023

tustvold Apr 20, 2023

Weijun-H commented Apr 20, 2023

alamb left a comment

alamb Apr 22, 2023

alamb Apr 22, 2023

alamb Apr 22, 2023

alamb left a comment

alamb Apr 24, 2023

alamb Apr 24, 2023

alamb Apr 24, 2023

Weijun-H Apr 24, 2023

alamb left a comment

feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

Conversation

Weijun-H commented Apr 19, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Weijun-H commented Apr 20, 2023

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb left a comment

Choose a reason for hiding this comment