Jefffrey commented on code in PR #12043:
URL: https://github.com/apache/arrow-rs/pull/12043#discussion_r4464483449
##########
arrow-row/src/lib.rs:
##########
@@ -2262,6 +2300,36 @@ pub fn encode_dictionary_values<K:
ArrowDictionaryKeyType>(
}
}
+fn gather_dictionary_values<K: ArrowDictionaryKeyType>(
Review Comment:
using take here could be a good idea, especially regarding not copying
unnecessary data for null slots (e.g. in case element 0 is a large string)
though it does force bringing in `arrow-select` as a dependency 🤔
##########
arrow-row/src/lib.rs:
##########
@@ -775,7 +781,20 @@ impl Codec {
match self {
Codec::Stateless => Ok(Encoder::Stateless),
Codec::Dictionary(converter, nulls) => {
- let values = array.as_any_dictionary().values().clone();
+ let dictionary = array.as_any_dictionary();
+ let values = dictionary.values();
+ if values.len() >= DICTIONARY_GATHER_MIN_VALUES
+ && values.len() >=
dictionary.len().saturating_mul(DICTIONARY_GATHER_RATIO)
Review Comment:
should we also set a limit to how much we hydrate? in case we run into
offset overflows?
identified by codex, see full output:
<details>
Gathering can reject valid inputs through offset overflow — lib.rs:790
(arrow-row/src/lib.rs:790). Materializing repeated values can exceed the value
type’s limits. I reproduced this with dictionary values of type
RunEndEncoded<Int64, RunEndEncoded<Int16, Int32>>: 131,072 logical values and
32,768 repeated keys produce a run-end overflow. The existing path succeeds
with the same logical output. Fall back to dictionary encoding when gathering
overflows.
</details>
- it does seem like quite an extreme edge case
##########
arrow-row/src/lib.rs:
##########
@@ -911,6 +930,8 @@ enum Encoder<'a> {
Stateless,
/// The encoding of the child array and the encoding of a null row
Dictionary(Rows, Row<'a>),
+ /// The encoding of one dictionary value per input row and a null row
Review Comment:
```suggestion
/// The encoding of the hydrated dictionary array and a null row
```
or `flattened dictionary array` maybe? just to make it more clear we're
essentially expanding the dictionary out to a regular 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
