You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2021/04/16 17:22:12 UTC
[GitHub] [arrow] tustvold opened a new pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
tustvold opened a new pull request #10073:
URL: https://github.com/apache/arrow/pull/10073
Currently calling concat on two dictionary encoded arrays blinding concatenates the keys arrays, and keeps only the dictionary of the first. This can lead to invalid data, including keys referring to value indexes beyond the bounds of the values array.
This PR alters MutableArrayData to concatenate the child data of any dictionary arrays passed to it, and offset the source keys to correctly map to the relevant "slice" of the resulting child data. This does mean that the resulting dictionary array may contain duplicates, and will not be sorted, but I'm not sure if this is a problem?
Signed-off-by: Raphael Taylor-Davies <r....@googlemail.com>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615374353
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
+ let concat_collected = collect_string_dictionary(concat);
+ let input_1_collected = collect_string_dictionary(&input_1);
+ let input_2_collected = collect_string_dictionary(&input_2);
+ let expected: Vec<_> = input_1_collected
+ .into_iter()
+ .chain(input_2_collected.into_iter())
+ .collect();
+
+ assert_eq!(concat_collected, expected);
+
+ Ok(())
Review comment:
I suggest adding at least one Null value to the dictionary to test the nulls case
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
Review comment:
I recommend testing the actual values from the dictionary array (aka test the content is as expected) as well here explicitly -- something like
```
let expected_str = vec![
"hello", "A", "B", "hello", "hello", "C",
"hello", "E", "E", "hello", "F", "E",
];
let values = concat.values();
let values = values
.as_any()
.downcast_ref::<StringArray>()
.unwrap();
let concat_str = concat.keys().iter().map(|key| {
let key = key.unwrap();
values.value(key as usize)
})
.collect::<Vec<&str>>();
assert_eq!(expected_str, concat_str);
```
(tested and it passes for me locally)
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
Review comment:
I think you can probably iterate over the keys directly if you wanted:
```suggestion
dictionary.keys()
.iter()
.map(|key| {
key.map(|key| values.value(key as _).to_string())
})
.collect()
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#issuecomment-821323430
https://issues.apache.org/jira/browse/ARROW-12426
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] tustvold commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615649844
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
Review comment:
I'm confused, is this not what I do a couple of lines down?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] tustvold commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615649844
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
Review comment:
I'm confused, is this not what I do a couple of lines down?
Edit: Oh I see what you're saying
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#issuecomment-822357108
The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories [arrow-rs](https://github.com/apache/arrow-rs) and [arrow-datafusion](https://github.com/apache/arrow-datafusion). It is likely we will not merge this PR into this repository
Please see the [mailing-list](https://lists.apache.org/thread.html/rce7c61cb3f703367dc00984530016e3fcb828e5a8035655fbcccf862%40%3Cdev.arrow.apache.org%3E) thread for more details
We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb removed a comment on pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
alamb removed a comment on pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#issuecomment-822358110
The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories [arrow-rs](https://github.com/apache/arrow-rs) and [arrow-datafusion](https://github.com/apache/arrow-datafusion). It is likely we will not merge this PR into this repository
Please see the [mailing-list](https://lists.apache.org/thread.html/rce7c61cb3f703367dc00984530016e3fcb828e5a8035655fbcccf862%40%3Cdev.arrow.apache.org%3E) thread for more details
We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] tustvold commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615649844
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
Review comment:
I'm confused, is this not what I do a couple of lines down?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615736126
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
Review comment:
> Edit: I've taken a guess at what you meant and hardcoded the expected result instead of computing it
Yes, I meant a hard coded expected result.
The rationale being that then no change or bug in the arrow implementation can cause the expected results to change. By comparing the output of `concat` (which calls `concat_dictionary`) to `concat_dictionary` I was thinking that a bug in `concat_dictionary` could be masked in some cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] tustvold closed pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
tustvold closed pull request #10073:
URL: https://github.com/apache/arrow/pull/10073
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#issuecomment-821967101
(and there is a clippy failure on this PR :()
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
alamb commented on pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#issuecomment-822358110
The Apache Arrow Rust community is moving the Rust implementation into its own dedicated github repositories [arrow-rs](https://github.com/apache/arrow-rs) and [arrow-datafusion](https://github.com/apache/arrow-datafusion). It is likely we will not merge this PR into this repository
Please see the [mailing-list](https://lists.apache.org/thread.html/rce7c61cb3f703367dc00984530016e3fcb828e5a8035655fbcccf862%40%3Cdev.arrow.apache.org%3E) thread for more details
We expect the process to take a few days and will follow up with a migration plan for the in-flight PRs.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] tustvold commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615649844
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
Review comment:
I'm confused, is this not what I do a couple of lines down?
Edit: I've taken a guess at what you meant and hardcoded the expected result instead of computing it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] tustvold commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
tustvold commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615010933
##########
File path: rust/arrow/src/array/transform/mod.rs
##########
@@ -353,7 +429,23 @@ impl<'a> MutableArrayData<'a> {
let null_bytes = bit_util::ceil(capacity, 8);
let null_buffer = MutableBuffer::from_len_zeroed(null_bytes);
- let extend_values = arrays.iter().map(|array| build_extend(array)).collect();
+ let extend_values = match &data_type {
+ DataType::Dictionary(_, _) => {
+ let mut next_offset = 0;
+ let extend_values: Result<Vec<_>> = arrays
+ .iter()
+ .map(|array| {
+ let offset = next_offset;
+ next_offset += array.child_data()[0].len();
+ Ok(build_extend_dictionary(array, offset, next_offset)
+ .ok_or(ArrowError::DictionaryKeyOverflowError)?)
+ })
+ .collect();
+
+ extend_values.expect("MutableArrayData::new is infallible")
Review comment:
I'm not sure how best to handle this, the other option would be to handle dictionary concatenation in the concat kernel which is fallible - not sure which is better as I'm not all that familiar with the codebase yet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] tustvold commented on pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
tustvold commented on pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#issuecomment-823949185
Will retarget to the new repository
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] alamb commented on a change in pull request #10073: ARROW-12426: [Rust] Fix concatentation of arrow dictionaries
Posted by GitBox <gi...@apache.org>.
alamb commented on a change in pull request #10073:
URL: https://github.com/apache/arrow/pull/10073#discussion_r615736126
##########
File path: rust/arrow/src/compute/kernels/concat.rs
##########
@@ -384,4 +384,51 @@ mod tests {
Ok(())
}
+
+ fn collect_string_dictionary(
+ dictionary: &DictionaryArray<Int32Type>,
+ ) -> Vec<Option<String>> {
+ let values = dictionary.values();
+ let values = values.as_any().downcast_ref::<StringArray>().unwrap();
+
+ (0..dictionary.len())
+ .map(move |i| match dictionary.keys().is_valid(i) {
+ true => {
+ let key = dictionary.keys().value(i);
+ Some(values.value(key as _).to_string())
+ }
+ false => None,
+ })
+ .collect()
+ }
+
+ #[test]
+ fn test_string_dictionary_array() -> Result<()> {
+ let input_1: DictionaryArray<Int32Type> =
+ vec!["hello", "A", "B", "hello", "hello", "C"]
+ .into_iter()
+ .collect();
+ let input_2: DictionaryArray<Int32Type> =
+ vec!["hello", "E", "E", "hello", "F", "E"]
+ .into_iter()
+ .collect();
+
+ let concat = concat(&[&input_1 as _, &input_2 as _]).unwrap();
+ let concat = concat
+ .as_any()
+ .downcast_ref::<DictionaryArray<Int32Type>>()
+ .unwrap();
+
Review comment:
> Edit: I've taken a guess at what you meant and hardcoded the expected result instead of computing it
Yes, I meant a hard coded expected result.
The rationale being that then there is less change a bug in the arrow implementation can cause the expected results to change. By comparing the output of `concat` (which calls `concat_dictionary`) to `concat_dictionary` I was thinking that a bug in `concat_dictionary` could be masked in some cases
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org