scovich commented on code in PR #8514:
URL: https://github.com/apache/arrow-rs/pull/8514#discussion_r2391382760
##########
parquet-variant/src/variant/list.rs:
##########
@@ -117,7 +117,7 @@ impl VariantListHeader {
///
/// [valid]: VariantMetadata#Validation
/// [Variant spec]:
https://github.com/apache/parquet-format/blob/master/VariantEncoding.md#value-data-for-array-basic_type3
-#[derive(Debug, Clone, PartialEq)]
Review Comment:
We forgot to fix this `PartialEq` when we fixed the one for `VariantObject`.
Manual implementation below.
##########
parquet-variant/src/variant/object.rs:
##########
@@ -419,13 +419,9 @@ impl<'m, 'v> PartialEq for VariantObject<'m, 'v> {
// IFF two objects are valid and logically equal, they will have the
same
// field names in the same order, because the spec requires the object
// fields to be sorted lexicographically.
- for ((name_a, value_a), (name_b, value_b)) in
self.iter().zip(other.iter()) {
- if name_a != name_b || value_a != value_b {
- return false;
- }
- }
-
- true
+ self.iter()
+ .zip(other.iter())
+ .all(|((name_a, value_a), (name_b, value_b))| name_a == name_b &&
value_a == value_b)
Review Comment:
Opportunistic simplification.
##########
parquet/tests/variant_integration.rs:
##########
@@ -34,24 +34,16 @@ use std::{fs, path::PathBuf};
type Result<T> = std::result::Result<T, String>;
-/// Creates a test function for a given case number
+/// Creates a test function for a given case number.
+///
+/// If an error message is provided, generate an error test case that expects
it.
///
/// Note the index is zero-based, while the case number is one-based
macro_rules! variant_test_case {
- ($case_num:literal) => {
- paste::paste! {
- #[test]
- fn [<test_variant_integration_case_ $case_num>]() {
- all_cases()[$case_num - 1].run()
- }
- }
- };
-
- // Generates an error test case, where the expected result is an error
message
- ($case_num:literal, $expected_error:literal) => {
Review Comment:
Opportunistic simplification: Instead of duplicating the macro definition,
just use `$(...)?` syntax to capture (and respond to) an optional error message.
##########
parquet-variant-compute/src/unshred_variant.rs:
##########
@@ -17,10 +17,12 @@
//! Module for unshredding VariantArray by folding typed_value columns back
into the value column.
+use crate::arrow_to_variant::ListLikeArray;
Review Comment:
Thanks for the nice new interface, @liamzwbao !
--
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]
