Improve performance of DictionaryArray::try_new() #1435

jackwener · 2022-03-13T14:25:08Z

Which issue does this PR close?

Closes #1313.

What changes are included in this PR?

extract the dictionary validation logic out of ArrayData::validate_full into a separate function.

DictionaryArray::try_new use the ArrayDataBuilder::build_unchecked and afterwards call the new function which only validates that the keys are in bounds.

Are there any user-facing changes?

None

codecov-commenter · 2022-03-13T14:36:48Z

Codecov Report

Merging #1435 (b9b79cf) into master (d9099c4) will decrease coverage by 0.00%.
The diff coverage is 89.28%.

@@            Coverage Diff             @@
##           master    #1435      +/-   ##
==========================================
- Coverage   82.68%   82.67%   -0.01%     
==========================================
  Files         185      185              
  Lines       53822    53825       +3     
==========================================
- Hits        44502    44501       -1     
- Misses       9320     9324       +4

Impacted Files	Coverage Δ
arrow/src/array/data.rs	`82.82% <88.00%> (-0.34%)`	⬇️
arrow/src/array/array_dictionary.rs	`91.73% <100.00%> (+0.07%)`	⬆️
arrow/src/datatypes/field.rs	`53.79% <0.00%> (-0.31%)`	⬇️
arrow/src/array/transform/mod.rs	`86.31% <0.00%> (-0.12%)`	⬇️
arrow/src/array/array_list.rs	`95.52% <0.00%> (ø)`
parquet/src/encodings/encoding.rs	`93.71% <0.00%> (+0.19%)`	⬆️
parquet_derive/src/parquet_field.rs	`66.43% <0.00%> (+0.22%)`	⬆️

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 d9099c4...b9b79cf. Read the comment docs.

viirya · 2022-03-13T22:49:15Z

arrow/src/array/data.rs

+                    DataType::Int8 => self.check_bounds::<i8>(max_value),
+                    DataType::Int16 => self.check_bounds::<i16>(max_value),
+                    DataType::Int32 => self.check_bounds::<i32>(max_value),
+                    DataType::Int64 => self.check_bounds::<i64>(max_value),
                    _ => unreachable!(),


I think a possible solution would be to extract the dictionary validation logic out of ArrayData::validate_full into a separate function. DictionaryArray::try_new could then use ArrayDataBuilder::build_unchecked and afterwards call the new function which only validates that the keys are in bounds.

I think "the dictionary validation logic" is only for the logic inside DataType::Dictionary pattern branch.

The validation logic for other data types are not for dictionary offset.

viirya · 2022-03-13T22:55:08Z

arrow/src/array/array_dictionary.rs

+        let array = unsafe { data.build_unchecked() };
+
+        array.validate_dictionary_offest()?;


Actually validate_full also contains "cheap" validation (validate), like buffer length, key type, etc. I think they seems necessary still.

I think the idea is that since the input to this function is a valid array, then the array data itself is also valid.

Thus, we know that the dictionary keys are valid integers of type K but we don't know that they are all within the range 0..dictionary_values.len() so that validation is still required. I think this PR makes this change

Hmm, we only have a limit that K: ArrowPrimitiveType. But in validate, it will further check if K is dictionary key type by DataType::is_dictionary_key_type, so I think the check is missed after this change?

That is an excellent point @viirya -- perhaps we can add a test case that tries to do something crazy like use Float64 keys to verify.

Perhaps the test could look like this (untested):

#[test] #[should_panic( expected = "Type is not valid dictionary type" )] fn test_try_new_index_too_large() { let values: StringArray = [Some("foo"), Some("bar")].into_iter().collect(); let keys: Float32Array = [Some(0), None, Some(3)].into_iter().collect(); DictionaryArray::<Float32Type>::try_new(&keys, &values).unwrap(); }

@viirya are you suggesting we call array.validate() in addition to array.validate_dictionary_offest()? If so that makes sense to me

Yes, looks like we also need validate in additional to validate_dictionary_offest.

alamb

Thank you @jackwener ! I think this looks good. 👍

Could you also perhaps update the comments in try_new to reflect what you have changed? For example this is likely no longer relevant:

        // Note: This does more work than necessary by rebuilding /
        // revalidating all the data

arrow/src/array/data.rs

alamb

I also double checked and there is already test coverage for this scenario:

https://github.com/apache/arrow-rs/blob/master/arrow/src/array/array_dictionary.rs#L557-L577

👍

alamb · 2022-03-16T07:51:13Z

arrow/src/array/array_dictionary.rs

+        let array = unsafe { data.build_unchecked() };
+
+        array.validate_dictionary_offest()?;


I think the idea is that since the input to this function is a valid array, then the array data itself is also valid.

Thus, we know that the dictionary keys are valid integers of type K but we don't know that they are all within the range 0..dictionary_values.len() so that validation is still required. I think this PR makes this change

jackwener · 2022-03-22T07:09:47Z

I have read the dialogue above. thanks @viirya @alamb ❤😃.
I add the cheap validate() and a unit test for wrong dictionary key.

alamb · 2022-03-22T10:21:55Z

arrow/src/array/array_dictionary.rs

@@ -112,7 +113,12 @@ impl<'a, K: ArrowPrimitiveType> DictionaryArray<K> {
            _ => data = data.null_count(0),
        }

-        Ok(data.build()?.into())
+        let array = unsafe { data.build_unchecked() };


Suggested change

let array = unsafe { data.build_unchecked() };

// Safety: `validate` ensures key type is correct, and

// `validate_dictionary_offset` ensures all offsets are within range

let array = unsafe { data.build_unchecked() };

alamb · 2022-03-22T10:22:32Z

Looks good -- thank you @jackwener and @viirya

viirya · 2022-03-22T16:12:07Z

lgtm

github-actions bot added the arrow Changes to the arrow crate label Mar 13, 2022

viirya reviewed Mar 13, 2022

View reviewed changes

alamb reviewed Mar 16, 2022

View reviewed changes

arrow/src/array/data.rs Outdated Show resolved Hide resolved

alamb approved these changes Mar 16, 2022

View reviewed changes

jackwener added 3 commits March 22, 2022 15:07

improve DictionaryArray::try_new() apache#1313

b326af6

*: fix typo

c788636

*: add cheap validate and unit test

a16ce31

jackwener force-pushed the check branch from 61963ba to 5070abb Compare March 22, 2022 07:10

*: polish the error

c661156

jackwener force-pushed the check branch from 5070abb to c661156 Compare March 22, 2022 07:18

alamb reviewed Mar 22, 2022

View reviewed changes

Add safety note

eaa0810

alamb approved these changes Mar 22, 2022

View reviewed changes

alamb merged commit 3ee2e67 into apache:master Mar 22, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve performance of DictionaryArray::try_new() #1435

Improve performance of DictionaryArray::try_new() #1435

jackwener commented Mar 13, 2022

codecov-commenter commented Mar 13, 2022

viirya Mar 13, 2022

viirya Mar 13, 2022

viirya Mar 13, 2022

alamb Mar 16, 2022

viirya Mar 16, 2022

alamb Mar 16, 2022

alamb Mar 16, 2022

viirya Mar 16, 2022

alamb left a comment

alamb left a comment

alamb Mar 16, 2022

jackwener commented Mar 22, 2022 •

edited

Loading

alamb Mar 22, 2022

alamb commented Mar 22, 2022

viirya commented Mar 22, 2022

		let array = unsafe { data.build_unchecked() };

		array.validate_dictionary_offest()?;

Improve performance of DictionaryArray::try_new() #1435

Improve performance of DictionaryArray::try_new() #1435

Conversation

jackwener commented Mar 13, 2022

Which issue does this PR close?

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Mar 13, 2022

Codecov Report

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

Choose a reason for hiding this comment

alamb left a 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

jackwener commented Mar 22, 2022 • edited Loading

Choose a reason for hiding this comment

alamb commented Mar 22, 2022

viirya commented Mar 22, 2022

jackwener commented Mar 22, 2022 •

edited

Loading