You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "klion26 (via GitHub)" <gi...@apache.org> on 2025/07/06 07:30:46 UTC
[PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
klion26 opened a new pull request, #7876:
URL: https://github.com/apache/arrow-rs/pull/7876
# Which issue does this PR close?
Add tests for large variant list.
- Closes #7820 .
# Rationale for this change
Add tests for larget vairant lists
# What changes are included in this PR?
This PR adds three tests for large variant lists.
- one for total child size between 2^8 and 2^16
- one for total child size between 2^16 and 2^24
- one for total child size between 2^24 and 2^32
all the tests will verify the `is_large`, `offset_size` and the content of the list.
# Are these changes tested?
Yes
# Are there any user-facing changes?
Non
--
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
Re: [PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #7876:
URL: https://github.com/apache/arrow-rs/pull/7876#issuecomment-3048177842
@alamb, thanks for the review. I've addressed the comment. Please take another look when you're free. Thank you.
--
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
Re: [PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #7876:
URL: https://github.com/apache/arrow-rs/pull/7876#discussion_r2191069707
##########
parquet-variant/src/variant/list.rs:
##########
@@ -410,4 +412,170 @@ mod tests {
let elem1 = variant_list.get(1).unwrap();
assert_eq!(elem1.as_boolean(), Some(false));
}
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_8_and_2_pow_16() {
+ // all the tests below will set the total child size to ~500,
+ // which is larger than 2^8 but less than 2^16.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 500;
+
+ test_large_variant_list_with_child_length(
+ list_size, // the elements in the list
+ single_child_item_len, // this will control the total child size in the list
+ OffsetSizeBytes::Two,
+ );
+
+ list_size = 255;
+ single_child_item_len = 2;
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Two,
+ );
+
+ list_size = 256;
+ single_child_item_len = 2;
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Two,
+ );
+
+ list_size = 300;
+ single_child_item_len = 2;
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Two,
+ );
+ }
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_16_and_2_pow_24() {
+ // all the tests below will set the total child size to ~70,000,
+ // which is larger than 2^16 but less than 2^24.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 70000;
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Three,
+ );
+
+ list_size = 255;
+ single_child_item_len = 275;
+ // total child size = 255 * 275 = 70,125
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Three,
+ );
+
+ list_size = 256;
+ single_child_item_len = 274;
+ // total child size = 256 * 274 = 70,144
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Three,
+ );
+
+ list_size = 300;
+ single_child_item_len = 234;
+ // total child size = 300 * 234 = 70,200
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Three,
+ );
+ }
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_24_and_2_pow_32() {
+ // all the tests below will set the total child size to ~20,000,000,
+ // which is larger than 2^24 but less than 2^32.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 20000000;
+
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Four,
+ );
+
+ list_size = 255;
+ single_child_item_len = 78432;
+ // total child size = 255 * 78,432 = 20,000,160
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Four,
+ );
+
+ list_size = 256;
+ single_child_item_len = 78125;
+ // total child size = 256 * 78,125 = 20,000,000
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Four,
+ );
+
+ list_size = 300;
+ single_child_item_len = 66667;
+ // total child size = 300 * 66,667 = 20,000,100
+ test_large_variant_list_with_child_length(
+ list_size,
+ single_child_item_len,
+ OffsetSizeBytes::Four,
+ );
+ }
+
+ // this function will create a large variant list from VariantBuilder
+ // with specified size and each child item with the given length.
+ // and verify the content and some meta for the variant list in the final.
+ fn test_large_variant_list_with_child_length(
+ list_size: usize,
+ single_child_item_len: usize,
+ expected_offset_size_bytes: OffsetSizeBytes,
+ ) {
+ let mut builder = VariantBuilder::new();
+ let mut list_builder = builder.new_list();
+
+ let mut expected_list = vec![];
+ for i in 0..list_size {
+ let random_string: String =
+ repeat_n(char::from((i % 256) as u8), single_child_item_len).collect();
+
+ list_builder.append_value(Variant::String(random_string.as_str()));
+ expected_list.push(random_string);
+ }
+
+ list_builder.finish();
+ // Finish the builder to get the metadata and value
+ let (metadata, value) = builder.finish();
+ // use the Variant API to verify the result
+ let variant = Variant::try_new(&metadata, &value).unwrap();
+
+ let variant_list = variant.as_list().unwrap();
+
+ // verify that the head is expected
+ assert_eq!(expected_offset_size_bytes, variant_list.header.offset_size);
Review Comment:
this is very nice
##########
parquet-variant/src/variant/list.rs:
##########
@@ -410,4 +412,170 @@ mod tests {
let elem1 = variant_list.get(1).unwrap();
assert_eq!(elem1.as_boolean(), Some(false));
}
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_8_and_2_pow_16() {
+ // all the tests below will set the total child size to ~500,
+ // which is larger than 2^8 but less than 2^16.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 500;
Review Comment:
It looks like you can check the is_large using this API
https://github.com/apache/arrow-rs/blob/c65058cfdd3ebb8e63e6e7c5a9cbe8a6ac93f75e/parquet-variant/src/variant/list.rs#L41-L40
Something like this (with the appropriate OffsetSizeBytes)
```
assert_eq!(variant_list.header.offset_size, OffsetSizeBytes::Two);
```
--
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
Re: [PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #7876:
URL: https://github.com/apache/arrow-rs/pull/7876#discussion_r2191986503
##########
parquet-variant/src/variant/list.rs:
##########
@@ -410,4 +412,170 @@ mod tests {
let elem1 = variant_list.get(1).unwrap();
assert_eq!(elem1.as_boolean(), Some(false));
}
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_8_and_2_pow_16() {
+ // all the tests below will set the total child size to ~500,
+ // which is larger than 2^8 but less than 2^16.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 500;
Review Comment:
Ok, added 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.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #7876:
URL: https://github.com/apache/arrow-rs/pull/7876#discussion_r2191986503
##########
parquet-variant/src/variant/list.rs:
##########
@@ -410,4 +412,170 @@ mod tests {
let elem1 = variant_list.get(1).unwrap();
assert_eq!(elem1.as_boolean(), Some(false));
}
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_8_and_2_pow_16() {
+ // all the tests below will set the total child size to ~500,
+ // which is larger than 2^8 but less than 2^16.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 500;
Review Comment:
Ok, will add this
--
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
Re: [PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #7876:
URL: https://github.com/apache/arrow-rs/pull/7876#issuecomment-3041099606
@alamb Please help review this when you're free, thanks.
--
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
Re: [PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on code in PR #7876:
URL: https://github.com/apache/arrow-rs/pull/7876#discussion_r2189043032
##########
parquet-variant/src/variant/list.rs:
##########
@@ -410,4 +412,170 @@ mod tests {
let elem1 = variant_list.get(1).unwrap();
assert_eq!(elem1.as_boolean(), Some(false));
}
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_8_and_2_pow_16() {
+ // all the tests below will set the total child size to ~500,
+ // which is larger than 2^8 but less than 2^16.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 500;
Review Comment:
Before I want to update the `list_size` to verify the `is_large` in metadata, now the `is_large` is removed, I can simplify this test to a single case(single `list_size` and `single_child_item_len`) if needed.
##########
parquet-variant/src/variant/list.rs:
##########
@@ -410,4 +412,170 @@ mod tests {
let elem1 = variant_list.get(1).unwrap();
assert_eq!(elem1.as_boolean(), Some(false));
}
+
+ #[test]
+ fn test_large_variant_list_with_total_child_length_between_2_pow_8_and_2_pow_16() {
+ // all the tests below will set the total child size to ~500,
+ // which is larger than 2^8 but less than 2^16.
+ // total child size = list_size * single_child_item_len
+
+ let mut list_size: usize = 1;
+ let mut single_child_item_len: usize = 500;
Review Comment:
Before I want to update the `list_size` to verify the `is_large` in metadata, now the `is_large` has been removed, I can simplify this test to a single case(single `list_size` and `single_child_item_len`) if needed.
--
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
Re: [PR] [ARROW-RS-7820][Variant] Add tests for large variant lists [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #7876:
URL: https://github.com/apache/arrow-rs/pull/7876
--
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