Support dictionary encoding in structures for `FlightDataEncoder`, add documentation for `arrow_flight::encode::Dictionary` #5488

thinkharderdev · 2024-03-09T14:32:11Z

…Handling

Which issue does this PR close?

Closes #5487

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

…Handling

thinkharderdev · 2024-03-09T14:33:07Z

arrow-flight/src/encode.rs

@@ -388,29 +388,39 @@ impl Stream for FlightDataEncoder {
 /// Defines how a [`FlightDataEncoder`] encodes [`DictionaryArray`]s
 ///
 /// [`DictionaryArray`]: arrow_array::DictionaryArray
+///


This is how things work in arrow-rs and I'm just assuming this is how the flight protocol is supposed to work :)

Should we update this comment maybe to say that the FlightDataEncoder doesn't really handle sending the same dictionary multiple times?

doesn't really handle as in will send the same dictionary over and over again for evert batch?

Added

/// The current implementation does not support "delta" dictionaries so a new dictionary batch will be sent each time the encoder sees a /// dictionary which is not pointer-equal to the previously observed dictionary for a given `dict_id`.

to clarify that we do not send delta dictionaries but the current implementation will actually skip sending the dictionary if it is pointer-equal to existing tracked dictionary.

alamb

Thank you @thinkharderdev -- I think there is still a misunderstanding somewhere as this new text doesn't match what I think the code does

alamb · 2024-03-09T16:31:44Z

arrow-flight/src/encode.rs

    ///
-    /// See also:
-    /// * <https://github.com/apache/arrow-rs/issues/1206>
+    /// The arrow flight protocol supports delta dictionaries where the sender can send new dictionary values


This matches my understanding too

alamb · 2024-03-09T16:39:17Z

arrow-flight/src/encode.rs

-    /// An IPC response, streaming or otherwise, defines its schema up front
-    /// which defines the mapping from dictionary IDs. It then sends these
-    /// dictionaries over the wire.
+    /// This method should be used if all batches over the entire lifetime fo the flight stream


I don't think this description is accurate. The behavior is what is "supposed" to happen with Flight, but to the best of my knowledge, this behavior isn't implemented -- the gap is tracked in #3389 but I don't think it is yet implemented

I think what Hydrate actually does is to send arrays using the underlying type -- for example, DictionaryArray<Int32, Utf8> would be cast to StringArray and the StringArray would be sent over the network

I believe Resend actually resends a new dictionary for each DictionaryArray, without trying to figure out if it was the same as the previous dictionary

Hmm, I think you're right but the current implementation is actually broken then because it doesn't take into account nested dictionary fields:

fn prepare_schema_for_flight(schema: &Schema, send_dictionaries: bool) -> Schema { let fields: Fields = schema .fields() .iter() .map(|field| match field.data_type() { DataType::Dictionary(_, value_type) if !send_dictionaries => Field::new( field.name(), value_type.as_ref().clone(), field.is_nullable(), ) .with_metadata(field.metadata().clone()), _ => field.as_ref().clone(), }) .collect(); Schema::new(fields).with_metadata(schema.metadata().clone()) }

Then if you have DictionaryHandling::Hydrate (the default) it will break on dictionary replacement of the nested field:

pub fn insert(&mut self, dict_id: i64, column: &ArrayRef) -> Result<bool, ArrowError> { let dict_data = column.to_data(); let dict_values = &dict_data.child_data()[0]; // If a dictionary with this id was already emitted, check if it was the same. if let Some(last) = self.written.get(&dict_id) { if ArrayData::ptr_eq(&last.child_data()[0], dict_values) { // Same dictionary values => no need to emit it again return Ok(false); } if self.error_on_replacement { // If error on replacement perform a logical comparison if last.child_data()[0] == *dict_values { // Same dictionary values => no need to emit it again return Ok(false); } return Err(ArrowError::InvalidArgumentError( "Dictionary replacement detected when writing IPC file format. \ Arrow IPC files only support a single dictionary for a given field \ across all batches." .to_string(), )); } } self.written.insert(dict_id, dict_data); Ok(true) }

Ok yeah, I think this is why I was getting so confused. The original description of Hydrate was correct but it just doesn't work correctly for nested dictionary arrays. So I guess we can

Fix the issue with nested fields

Revert back to original description

But I don't quite understand why Hydrate is even an option? It seems strictly worse than Resend? If you wanted to hydrate a dictionary array it seems like you would do that prior to sending over the flight stream instead of doing it on the fly in the protocol.

It seems strictly worse than Resend?

If you have a sparse dictionary, as you might if you've applied a selective predicate, sending a dictionary for each batch where most of the values are not used could conceivably be worse.

That being said this is also historical, as resend was added later

I agree it was mostly historical

The last time I checked (maybe 2 years ago) at least one client (maybe the golang one) didn't handle dictionaries (so if the rust server sent DictionaryArray the client couldn't deal with it) which was important for us as well

All in all, sorting out how to handle Dictionaries better / more rationally in arrow-flight would be a very nice improvement I think (aka working on #3389)

@alamb @tustvold Ok so I fixed handling of nested dictionary fields so Hydrate works in those cases now and restored the documentation back to more or less what it originally said (with some additional comments to clarify things a bit

…rate

alamb

Thank you @thinkharderdev -- I think this is an improvement over what is on main. I left some small suggestions as well but all in all thank you

alamb · 2024-03-13T17:30:19Z

arrow-flight/src/encode.rs

+    send_dictionaries: bool,
+) -> Result<ArrayRef> {
+    let arr = match (array.data_type(), data_type) {
+        (DataType::Dictionary(_, value), _) if !send_dictionaries => {


Is this code needed? It is strange to special case dictionary here, but just cast to data_type below. Maybe it is covered already by the (tpe, data_type) case

hmm, yeah I think you're right. We would have already changed the data type in the schema

Changed this and also renamed this method to hydrate_dictionaries since we only need to call when we have DictionaryHandling::Hydrate. In the case of Resend I just skip this entirely to avoid the pointless clones

alamb · 2024-03-13T17:31:04Z

arrow-flight/src/encode.rs

+
+    #[tokio::test]
+    async fn test_dictionary_struct_hydration() {
+        let struct_fields = vec![Field::new_list(


The amount of code required to build these nested arrays is quite sad (nothing wrong with this PR, I am just commenting in general)

yeah it is painful :)

alamb · 2024-03-13T17:32:18Z

arrow-flight/src/encode.rs

@@ -388,29 +388,39 @@ impl Stream for FlightDataEncoder {
 /// Defines how a [`FlightDataEncoder`] encodes [`DictionaryArray`]s
 ///
 /// [`DictionaryArray`]: arrow_array::DictionaryArray
+///


Should we update this comment maybe to say that the FlightDataEncoder doesn't really handle sending the same dictionary multiple times?

alamb

👌 very nice

Add more detailed documentation for arrow_flight::encode::Dicationary…

8a13eef

…Handling

thinkharderdev requested review from alamb and tustvold March 9, 2024 14:32

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels Mar 9, 2024

thinkharderdev commented Mar 9, 2024

View reviewed changes

fix doc link

b7a66d5

alamb changed the title ~~Add more detailed documentation for arrow_flight::encode::Dicationary…~~ Add more detailed documentation for arrow_flight::encode::Dictionary Mar 9, 2024

alamb reviewed Mar 9, 2024

View reviewed changes

alamb mentioned this pull request Mar 9, 2024

DataFusion weekly project plan (Andrew Lamb) - March 4, 2024 apache/datafusion#9453

Closed

5 tasks

Fix handling of nested dictionary arrays with DictionaryHandling::Hyd…

2799e7e

…rate

github-actions bot added the parquet Changes to the parquet crate label Mar 12, 2024

clippy

c4f37ec

thinkharderdev force-pushed the flight-docs branch from 6bd98e2 to c4f37ec Compare March 12, 2024 13:25

github-actions bot removed the parquet Changes to the parquet crate label Mar 12, 2024

thinkharderdev added 2 commits March 12, 2024 10:15

Handle large list and sparse unions

69c7912

use top-level fields

db37231

alamb changed the title ~~Add more detailed documentation for arrow_flight::encode::Dictionary~~ Support dictionary encoding in structures for FlightDataEncoder, add documentation for arrow_flight::encode::Dictionary Mar 13, 2024

alamb approved these changes Mar 13, 2024

View reviewed changes

PR comments

c8726b7

alamb approved these changes Mar 13, 2024

View reviewed changes

tustvold merged commit 14bd53d into apache:master Mar 15, 2024
12 checks passed

tustvold mentioned this pull request Mar 15, 2024

Improve documentation around handling of dictionary arrays in arrow flight #5487

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support dictionary encoding in structures for `FlightDataEncoder`, add documentation for `arrow_flight::encode::Dictionary` #5488

Support dictionary encoding in structures for `FlightDataEncoder`, add documentation for `arrow_flight::encode::Dictionary` #5488

thinkharderdev commented Mar 9, 2024

thinkharderdev Mar 9, 2024

alamb Mar 13, 2024

thinkharderdev Mar 13, 2024

thinkharderdev Mar 13, 2024

alamb left a comment

alamb Mar 9, 2024

alamb Mar 9, 2024

thinkharderdev Mar 9, 2024

thinkharderdev Mar 9, 2024 •

edited

Loading

tustvold Mar 9, 2024

alamb Mar 10, 2024

thinkharderdev Mar 12, 2024

alamb left a comment

alamb Mar 13, 2024

thinkharderdev Mar 13, 2024

thinkharderdev Mar 13, 2024

alamb Mar 13, 2024

thinkharderdev Mar 13, 2024

alamb Mar 13, 2024

alamb left a comment

Support dictionary encoding in structures for FlightDataEncoder, add documentation for arrow_flight::encode::Dictionary #5488

Support dictionary encoding in structures for FlightDataEncoder, add documentation for arrow_flight::encode::Dictionary #5488

Conversation

thinkharderdev commented Mar 9, 2024

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

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

thinkharderdev Mar 9, 2024 • edited Loading

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

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

Support dictionary encoding in structures for `FlightDataEncoder`, add documentation for `arrow_flight::encode::Dictionary` #5488

Support dictionary encoding in structures for `FlightDataEncoder`, add documentation for `arrow_flight::encode::Dictionary` #5488

thinkharderdev Mar 9, 2024 •

edited

Loading