Rich-T-kid commented on code in PR #11982:
URL: https://github.com/apache/arrow-rs/pull/11982#discussion_r4424876381
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2520,6 +2520,86 @@ mod tests {
writer.into_inner().unwrap()
}
+ #[test]
+ fn arrow_writer_string_dictionary_reused_across_batches() {
+ use arrow::compute::{cast, concat};
Review Comment:
place imports at the test mod level
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -85,26 +113,9 @@ macro_rules! downcast_op {
DataType::BinaryView => {
$op($array.as_any().downcast_ref::<BinaryViewArray>().unwrap()$(, $arg)*)
}
- DataType::Dictionary(key, value) => match value.as_ref() {
- DataType::Utf8 => downcast_dict_op!(key, StringArray, $array,
$op$(, $arg)*),
- DataType::LargeUtf8 => {
- downcast_dict_op!(key, LargeStringArray, $array, $op$(,
$arg)*)
- }
- DataType::Utf8View => {
- downcast_dict_op!(key, StringViewArray, $array, $op$(,
$arg)*)
- }
- DataType::Binary => downcast_dict_op!(key, BinaryArray,
$array, $op$(, $arg)*),
- DataType::LargeBinary => {
- downcast_dict_op!(key, LargeBinaryArray, $array, $op$(,
$arg)*)
- }
- DataType::BinaryView => {
- downcast_dict_op!(key, BinaryViewArray, $array, $op$(,
$arg)*)
- }
- DataType::FixedSizeBinary(_) => {
- downcast_dict_op!(key, FixedSizeBinaryArray, $array,
$op$(, $arg)*)
- }
- d => unreachable!("cannot downcast {} dictionary value to byte
array", d),
- },
+ DataType::Dictionary(key, value) => {
+ downcast_dict_values_op!(key, value, $array, $op$(, $arg)*)
+ }
Review Comment:
I dont think we are getting much value out of moving this macro around. can
we keep the match statement that was here
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -993,7 +1130,7 @@ fn count_within_budget_views(
// value by `max_value_len`. Skips the walk for the common small-value
// case (what view arrays are built for, and where there is nothing to
// bound).
- let per_value = max_value_len + std::mem::size_of::<u32>();
Review Comment:
I believe its conventional to leave the `std::mem`` prefix here. i'm
honestly not sure why but its common practice throughout this project so I'd
prefer to keep it the way it is.
##########
parquet/src/arrow/arrow_writer/byte_array.rs:
##########
@@ -598,13 +631,74 @@ impl DictEncoder {
Ok(())
}
+ /// Whether to encode the rows of an arrow dictionary with `dictionary` as
its values and
+ /// `num_rows` rows with [`Self::encode_dictionary`]
+ fn use_dictionary_cache(&self, dictionary: &ArrayRef, num_rows: usize) ->
bool {
+ // A new cache has a slot per dictionary value, which only pays off if
the values are
+ // referenced by enough rows
+ dictionary.len() <= num_rows
+ || self
+ .dictionary_cache
+ .as_ref()
+ .is_some_and(|cache| cache.is_for(dictionary))
+ }
+
+ /// Encode the rows of an arrow dictionary at `indices` to the in-progress
page
+ fn encode_dictionary<'a, K, V>(
Review Comment:
Could we rename `DictEncoder::encode_dictionary` to
`encode_with_dictionary_cache`? There are several things named
encode_dictionary, and this method specifically implements the cached Arrow-key
**to** Parquet-index path.
##########
parquet/src/arrow/arrow_writer/mod.rs:
##########
@@ -2520,6 +2520,86 @@ mod tests {
writer.into_inner().unwrap()
}
+ #[test]
+ fn arrow_writer_string_dictionary_reused_across_batches() {
Review Comment:
This test is doing a lot. Can we split it into two or three focused tests?
I’d separate cache reuse for the same values Arc, replacement when the
values Arc changes, and the cache-profitability policy. The last one should
probably test `use_dictionary_cache` directly, since an end-to-end round trip
cannot distinguish the cached and uncached paths.
--
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]
