feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

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

Which issue does this PR close?

Closes #3855

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

Why this change?

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

arrow-flight/src/encode.rs

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

    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 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

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

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

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

arrow-flight/src/encode.rs Outdated

Copy link

Contributor

tustvold Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this change?

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

-    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);
+    }

feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

feat: set FlightDescriptor on FlightDataEncoderBuilder #4101

Uh oh!

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?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Weijun-H commented Apr 20, 2023

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants