Re: [PR] Update `variant_integration` to use final series [arrow-rs]

["alamb (via GitHub)" <gi...@apache.org>]

alamb commented on code in PR #8325:
URL: https://github.com/apache/arrow-rs/pull/8325#discussion_r2343905650


##########
parquet-variant/src/variant/metadata.rs:
##########
@@ -309,6 +310,15 @@ impl<'m> VariantMetadata<'m> {
         self.header.offset_size.unpack_u32(bytes, i)
     }
 
+    /// Returns the total size, in bytes, of the metadata.
+    ///
+    /// Note this value may be smaller than what was passed to [`Self::new`] or
+    /// [`Self::try_new`] if the input was larger than necessary to encode the
+    /// metadata dictionary.
+    pub fn size(&self) -> usize {

Review Comment:
   I needed to expose this information because the variant metadata / data are appended in one .bin file in the test cases



##########
parquet/tests/variant_integration.rs:
##########
@@ -21,1233 +21,486 @@
 //! Variant values from .variant.bin files, reads Parquet files, converts StructArray
 //! to VariantArray, and verifies that extracted values match expected results.
 //!
-//! Based on the parquet-testing PR: https://github.com/apache/parquet-testing/pull/90/files
-//! Inspired by the arrow-go implementation: https://github.com/apache/arrow-go/pull/455/files
+//! Inspired by the arrow-go implementation: <https://github.com/apache/arrow-go/pull/455/files>
 
-// These tests require the arrow feature
-#![cfg(feature = "arrow")]
-
-use arrow_array::{Array, StructArray};
+use arrow::util::test_util::parquet_test_data;
+use arrow_array::{Array, ArrayRef};
+use arrow_cast::cast;
+use arrow_schema::{DataType, Fields};
 use parquet::arrow::arrow_reader::ParquetRecordBatchReaderBuilder;
-use std::{
-    env,
-    error::Error,
-    fs,
-    path::{Path, PathBuf},
-};
+use parquet_variant::{Variant, VariantMetadata};
+use parquet_variant_compute::VariantArray;
+use serde::Deserialize;
+use std::path::Path;
+use std::sync::{Arc, LazyLock};
+use std::{fs, path::PathBuf};
+
+type Result<T> = std::result::Result<T, String>;
+
+/// Creates a test function for a given case number
+///
+/// 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) => {
+        paste::paste! {
+            #[test]
+            #[should_panic(expected = $expected_error)]
+            fn [<test_variant_integration_case_ $case_num>]() {
+                all_cases()[$case_num - 1].run()
+            }
+        }
+    };
+}
 
-/// Test case definition structure matching the format from cases.json
-#[derive(Debug, Clone)]
+// Generate test functions for each case

Review Comment:
   I rewrote this file so the tests use the existing Rust test harness rather than our own. It does require some redundancy, but it means normal rust test execution tools work



##########
parquet/tests/variant_integration.rs:
##########
@@ -21,1233 +21,486 @@
 //! Variant values from .variant.bin files, reads Parquet files, converts StructArray
 //! to VariantArray, and verifies that extracted values match expected results.
 //!
-//! Based on the parquet-testing PR: https://github.com/apache/parquet-testing/pull/90/files
-//! Inspired by the arrow-go implementation: https://github.com/apache/arrow-go/pull/455/files
+//! Inspired by the arrow-go implementation: <https://github.com/apache/arrow-go/pull/455/files>
 
-// These tests require the arrow feature
-#![cfg(feature = "arrow")]
-
-use arrow_array::{Array, StructArray};
+use arrow::util::test_util::parquet_test_data;
+use arrow_array::{Array, ArrayRef};
+use arrow_cast::cast;
+use arrow_schema::{DataType, Fields};
 use parquet::arrow::arrow_reader::ParquetRecordBatchReaderBuilder;
-use std::{
-    env,
-    error::Error,
-    fs,
-    path::{Path, PathBuf},
-};
+use parquet_variant::{Variant, VariantMetadata};
+use parquet_variant_compute::VariantArray;
+use serde::Deserialize;
+use std::path::Path;
+use std::sync::{Arc, LazyLock};
+use std::{fs, path::PathBuf};
+
+type Result<T> = std::result::Result<T, String>;
+
+/// Creates a test function for a given case number
+///
+/// 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) => {
+        paste::paste! {
+            #[test]
+            #[should_panic(expected = $expected_error)]
+            fn [<test_variant_integration_case_ $case_num>]() {
+                all_cases()[$case_num - 1].run()
+            }
+        }
+    };
+}
 
-/// Test case definition structure matching the format from cases.json
-#[derive(Debug, Clone)]
+// Generate test functions for each case
+// Notes
+// - case 3 is empty in cases.json for some reason
+// - cases 40, 42, 87, 127 and 128 are expected to fail always (they include invalid variants)
+// - the remaining cases are expected to (eventually) pass
+
+variant_test_case!(1, "Unsupported typed_value type: List(");

Review Comment:
   I am quite pleased how many cases pass, actually



-- 
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