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 2022/08/23 18:46:52 UTC
[GitHub] [arrow-rs] psvri opened a new issue, #2564: Why do we check for null in TypedDictionaryArray value function
psvri opened a new issue, #2564:
URL: https://github.com/apache/arrow-rs/issues/2564
**Which part is this question about**
Code base: https://github.com/apache/arrow-rs/blob/master/arrow/src/array/array_dictionary.rs#L498.
**Describe your question**
In TypedDictionaryArray value function why do we check for null , we could instead assert if the index is less than keys.len() right?
**Additional context**
the trait documentation states the below, due to which I felt it was bizzare that we are checking for null.
```
/// A generic trait for accessing the values of an [`Array`]
pub trait ArrayAccessor: Array {
type Item: Send + Sync;
/// Returns the element at index `i`
/// # Panics
/// Panics if the value is outside the bounds of the array
fn value(&self, index: usize) -> Self::Item;
```
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1227178075
https://github.com/apache/arrow-rs/issues/2564 hopefully fixes this confusion, and fixes the OOB behaviour
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225993610
> Although in practice, I think that we won't put a out of boundary index value in a key element with null mask.
The only somewhat reasonable case would be an empty dictionary, where `0` would be out of bounds
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225991921
Thanks @psvri for catching it. As @tustvold said, it is a bug. `value_idx` may be out of boundary on `values`. Although in practice, I think that we won't put a out of boundary index value in a key element with null mask.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold closed issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
tustvold closed issue #2564: Why do we check for null in TypedDictionaryArray value function
URL: https://github.com/apache/arrow-rs/issues/2564
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225351143
There's a subtlety here, you get an **unspecified** value not an undefined value. It is not UB to read a null value from a ListArray, StringArray, etc... It isn't defined what you will get, but you won't get uninitialised memory or something that would trigger undefined behaviour.
TypedDictionaryArray is a special case because it is actually doing two array lookups. First it does a lookup into the indices array, if this index is null it gets back something unspecified. This is fine, however, this arbitrary value may not be in the bounds of the values, in which case we would access memory out of bounds.
We could also validate the bounds for the value access, however, the NULL assertion I thought more clearly indicated the why.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225268353
Dictionaries are a special case, as the value of a null index is undefined, we can't then use it as an index in the values array
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] psvri commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
psvri commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225867873
Thank you @tustvold for the explanation. I learnt a lot from it.
@tustvold @viirya
Just one last thing, based on your explanation, dosent this mean that we have actually implemented the function cmp_dict_primitive (https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/comparison.rs#L2263 ) wrongly and subsequently all the other kernels which use compare_op with a dictionary as one of its parameters.
As you can see in that function we are using compare_op to compare the elements of dictionary with a another array. And in compares op implementation (https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/comparison.rs#L65) we are using value_unchecked to get the elements of the dictionary without checking for null. So based on the above explanation we shouldnt be doing that right since the unspecified value can lead to out of bounds access ?
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225358980
> you get an unspecified value not an undefined value.
Make sense, Thank you @tustvold .
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] viirya commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
viirya commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1227552846
@tustvold I think you meant #2589.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225136120
I guess you are right @psvri. We always don't check the validation in the `value()` method, so the checking could be removed here.
cc @tustvold
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] HaoYang670 commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
HaoYang670 commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225274876
> Dictionaries are a special case, as the value of a null index is undefined, we can't then use it as an index in the values array.
Hmm🤔, Why is `Dictionary` a special case? The value under a null mask is always undefined for all kinds of arrays.
the processes are somewhat different:
For Dictionary, you will get an undefined `key` if `is_valid == false`, and then, you will use this undefined key to get an undefined `value`.
For other arrays (such as int8), you will get an undefined value if `is_valid == false`.
However, the results are same: You get an `undefined` value.
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on issue #2564: Why do we check for null in TypedDictionaryArray value function
Posted by GitBox <gi...@apache.org>.
tustvold commented on issue #2564:
URL: https://github.com/apache/arrow-rs/issues/2564#issuecomment-1225878750
> So based on the above explanation we shouldn't be doing that right since the unspecified value can lead to out of bounds access ?
Aah yeah, that's a bug. Fortunately it hasn't been released yet, it was changed in https://github.com/apache/arrow-rs/pull/2533. We should fix it before the next release.
I do wonder if there is a way to avoid needing to check the null index for dictionary arrays, perhaps we could enforce nulls to be 0, and then special case an empty dictionary or something... :thinking:
--
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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org