Fix `generate_nested_dictionary_case` integration test failure #1636

viirya · 2022-05-01T19:15:19Z

Which issue does this PR close?

Closes #1635.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

viirya · 2022-05-01T19:19:43Z

arrow/src/ipc/reader.rs

-        }
-    }
+    // Add (possibly multiple) array refs to the dictionaries array.
+    dictionaries_by_field.insert(id, dictionary_values.clone());


Main difference here. In create_array, we use dictionaries[node_index] to access dictionary array by node_index. But dictionary arrays are indexed by dict id, not node index.

viirya · 2022-05-01T19:20:39Z

integration-testing/src/lib.rs

@@ -640,6 +645,7 @@ fn dictionary_array_from_json(
    dict_key: &DataType,
    dict_value: &DataType,
    dictionary: &ArrowJsonDictionaryBatch,
+    dictionaries: Option<&HashMap<i64, ArrowJsonDictionaryBatch>>,


Passing in the map of dictionaries so nested dictionary can be used.

This is where the following error comes out when running archery --debug integration --run-flight --with-cpp=false --with-rust=true:

dictionary value type: List(Field { name: "str_dict", data_type: Dictionary(Int8, Utf8), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: None }) Error: JsonError("Unable to find any dictionaries for field Field { name: \"str_dict\", data_type: Dictionary(Int8, Utf8), nullable: true, dict_id: 0, dict_is_ordered: false, metadata: None }")

After fixing this, there is index error fixed by #1636 (review).

codecov-commenter · 2022-05-01T19:38:36Z

Codecov Report

Merging #1636 (7622bc3) into master (22e9f95) will increase coverage by 0.09%.
The diff coverage is 60.00%.

@@            Coverage Diff             @@
##           master    #1636      +/-   ##
==========================================
+ Coverage   83.02%   83.11%   +0.09%     
==========================================
  Files         193      193              
  Lines       55612    55847     +235     
==========================================
+ Hits        46174    46420     +246     
+ Misses       9438     9427      -11

Impacted Files	Coverage Δ
arrow-flight/src/utils.rs	`0.00% <0.00%> (ø)`
...ng/src/flight_client_scenarios/integration_test.rs	`0.00% <0.00%> (ø)`
...ng/src/flight_server_scenarios/integration_test.rs	`0.00% <0.00%> (ø)`
integration-testing/src/lib.rs	`0.00% <0.00%> (ø)`
arrow/src/ipc/reader.rs	`88.80% <88.23%> (-0.23%)`	⬇️
arrow/src/array/array.rs	`89.67% <0.00%> (-3.04%)`	⬇️
arrow/src/ffi.rs	`88.51% <0.00%> (ø)`
arrow/src/array/mod.rs	`100.00% <0.00%> (ø)`
arrow/src/array/builder.rs	`86.79% <0.00%> (ø)`
arrow/src/array/iterator.rs	`96.11% <0.00%> (ø)`
... and 13 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22e9f95...7622bc3. Read the comment docs.

tustvold

This makes sense to me, I think we should update the docs and naming to match the change 👍

tustvold · 2022-05-06T06:49:21Z

arrow/src/ipc/reader.rs

-            let value_array = dictionaries[node_index].clone().unwrap();
+
+            let value_array =
+                dictionaries.get(&field.dict_id().unwrap()).unwrap().clone();


We could maybe return an error here?

Yea, updated.

tustvold · 2022-05-06T06:52:57Z

arrow-flight/src/utils.rs

@@ -49,7 +50,7 @@ pub fn flight_data_from_arrow_batch(
 pub fn flight_data_to_arrow_batch(
    data: &FlightData,
    schema: SchemaRef,
-    dictionaries_by_field: &[Option<ArrayRef>],
+    dictionaries_by_field: &HashMap<i64, ArrayRef>,


I think this should be renamed to dictionaries_by_id, here and in all the other places?

Sounds good. I will update them.

alamb

Looks good to me -- thank you @viirya

BTW thank you so much for working through the integration test failures ❤️ so nice!

alamb · 2022-05-06T18:52:11Z

arrow/src/ipc/reader.rs

    data: &[u8],
    buffers: &[ipc::Buffer],
-    dictionaries: &[Option<ArrayRef>],
+    dictionaries: &HashMap<i64, ArrayRef>,


Suggested change

dictionaries: &HashMap<i64, ArrayRef>,

dictionaries_by_id: &HashMap<i64, ArrayRef>,

Maybe this would more consistent with the names used in the rest of this PR

alamb · 2022-05-06T18:53:48Z

arrow/src/ipc/reader.rs

@@ -457,7 +468,7 @@ pub fn read_record_batch(
    buf: &[u8],
    batch: ipc::RecordBatch,
    schema: SchemaRef,
-    dictionaries: &[Option<ArrayRef>],
+    dictionaries: &HashMap<i64, ArrayRef>,


Suggested change

dictionaries: &HashMap<i64, ArrayRef>,

dictionaries_by_id: &HashMap<i64, ArrayRef>,

viirya · 2022-05-06T19:10:44Z

Thank you @alamb. Renamed these names too.

Fix ipc nested dict

ae20913

github-actions bot added arrow Changes to the arrow crate arrow-flight Changes to the arrow-flight crate labels May 1, 2022

viirya changed the title ~~Fix ipc nested dict~~ Fix generate_nested_dictionary_case integration test failure for Rust cases May 1, 2022

viirya commented May 1, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/master' into fix_ipc_nested_dict

3ba6256

tustvold added the api-change Changes to the arrow API label May 6, 2022

tustvold reviewed May 6, 2022

View reviewed changes

Rename dictionaries_by_field.

f984dbf

alamb approved these changes May 6, 2022

View reviewed changes

Fix a few more inconsistent names

7622bc3

viirya force-pushed the fix_ipc_nested_dict branch from 0dcf24d to 7622bc3 Compare May 6, 2022 19:13

Rename a few more

3090a00

viirya merged commit 6ad893c into apache:master May 8, 2022

alamb changed the title ~~Fix generate_nested_dictionary_case integration test failure for Rust cases~~ Fix generate_nested_dictionary_case integration test failure May 12, 2022

alamb changed the title ~~Fix generate_nested_dictionary_case integration test failure~~ Fix generate_nested_dictionary_case integration test failure May 12, 2022

viirya mentioned this pull request May 12, 2022

Rust is not interoperability with C++ for IPC schemas with dictionaries #1694

Closed

alamb mentioned this pull request May 13, 2022

Update to arrow-rs 14.0.0 apache/datafusion#2528

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `generate_nested_dictionary_case` integration test failure #1636

Fix `generate_nested_dictionary_case` integration test failure #1636

viirya commented May 1, 2022

viirya May 1, 2022

viirya May 1, 2022

viirya May 1, 2022

codecov-commenter commented May 1, 2022 •

edited

Loading

tustvold left a comment

tustvold May 6, 2022

viirya May 6, 2022

tustvold May 6, 2022

viirya May 6, 2022

alamb left a comment

alamb May 6, 2022

alamb May 6, 2022

viirya commented May 6, 2022

	dictionaries: &HashMap<i64, ArrayRef>,
	dictionaries_by_id: &HashMap<i64, ArrayRef>,

Fix generate_nested_dictionary_case integration test failure #1636

Fix generate_nested_dictionary_case integration test failure #1636

Conversation

viirya commented May 1, 2022

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

codecov-commenter commented May 1, 2022 • edited Loading

Codecov Report

tustvold 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

viirya commented May 6, 2022

Fix `generate_nested_dictionary_case` integration test failure #1636

Fix `generate_nested_dictionary_case` integration test failure #1636

codecov-commenter commented May 1, 2022 •

edited

Loading