[VARIANT] Initial integration tests for variant reads #8104
Conversation
carpecodeum
force-pushed
the
tests-reading-variants
branch
from
5a6b96b to
3f38b82
Compare
|
Thank you @carpecodeum -- this is the first PR on my review queue tomorrow |
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you so much @carpecodeum and @mprammer. I played around with this PR locally and I think it is great.
I had a bunch of suggestions / ideas but I think we can potentially address them all as part of follow on PRs.
So here is my plan
- I'll merge this PR
- I'll make a PR to move this test to a new testing crate (where we can use the proper dependencies, etc)
- If you want, you can help the iterate in that new testing crate (e.g. actually loading Variants via
VariantArray)
Thanks again for getting this project rolling
There was a problem hiding this comment.
I don't think the simple in the name of the test adds much and thus I suggest using a simpler (🥁 ) name: variant_integration.rs
| //! Inspired by the arrow-go implementation: https://github.com/apache/arrow-go/pull/455/files | ||
|
|
||
| // These tests require the arrow feature | ||
| #![cfg(feature = "arrow")] |
There was a problem hiding this comment.
I think this only applies to the first arrow_array use statement
To declare a test requires a particular feature, I think you need to add a entry like this to Cargo.toml for simple_variant_integration
Lines 162 to 165 in 52fd59c
| } | ||
|
|
||
| // Individual test functions with #[ignore] for progressive enablement | ||
| // Following the exact pattern from the PR description |
| } | ||
|
|
||
| /// Load expected Variant binary data from .variant.bin file | ||
| #[allow(dead_code)] |
There was a problem hiding this comment.
I am pretty sure this function is not used, so maybe we can either remove it or change the code above to call it?
| #[ignore] // Enable once parquet-variant dependencies are added | ||
| fn test_variant_integration_case_001() { | ||
| let harness = VariantIntegrationHarness::new().expect("Failed to create test harness"); | ||
|
|
||
| let test_case = harness | ||
| .test_cases | ||
| .iter() | ||
| .find(|case| case.case_number == 1) | ||
| .expect("case-001 not found"); | ||
|
|
||
| harness | ||
| .run_single_test(test_case) | ||
| .expect("case-001 should pass"); | ||
| } |
There was a problem hiding this comment.
I think this pattern of creating one #test for each input case is very nice and results in easy to debug errors. However, since there will be so many cases (over 100 as I understand) I recommend we try and avoid as much repetition as possible
Could we write something like the following for each case? Then we could make a macro or something to generate all the functions
| #[ignore] // Enable once parquet-variant dependencies are added | |
| fn test_variant_integration_case_001() { | |
| let harness = VariantIntegrationHarness::new().expect("Failed to create test harness"); | |
| let test_case = harness | |
| .test_cases | |
| .iter() | |
| .find(|case| case.case_number == 1) | |
| .expect("case-001 not found"); | |
| harness | |
| .run_single_test(test_case) | |
| .expect("case-001 should pass"); | |
| } | |
| #[ignore] // Enable once parquet-variant dependencies are added | |
| fn test_variant_integration_case_001() { | |
| test_harness().run_case(1) | |
| } |
| } | ||
|
|
||
| /// Run a test case that should succeed | ||
| fn run_success_test(&self, test_case: &VariantTestCase) -> Result<(), Box<dyn Error>> { |
There was a problem hiding this comment.
In tests, I personally find it helpful to fail fast if there is a problem
Thus one pattern you might consider here returning () and using expect or unwrap in the test code
So something instead of
let struct_arrays = self.read_parquet_file(test_case)?;More like
let struct_arrays = self.read_parquet_file(test_case).expect("error reading parquet file");| println!(" Parquet file reading failed: {}", e); | ||
| if let Some(expected_error) = &test_case.error_message { | ||
| println!(" Expected error: {}", expected_error); | ||
| // TODO: Match actual error against expected error pattern |
There was a problem hiding this comment.
I am not quite sure if we want the rust implementation to match the same error messages as iceberg.
Another potential thing would be to call panic! here with the actual error message and then annotate the tests that fail with should_panic, for example here:
arrow-rs/arrow-array/src/array/struct_array.rs
Lines 644 to 646 in 13fc3c8
| parse_cases_json(&content) | ||
| } | ||
|
|
||
| /// Parse cases.json manually without serde |
There was a problem hiding this comment.
I don't see any reason we can't use serde_json here, but maybe we can do that as a follow on
| let _metadata = fs::read(&metadata_path)?; | ||
| let _value = fs::read(&value_path)?; | ||
|
|
||
| // TODO: Parse variant when parquet_variant crate is available |
There was a problem hiding this comment.
yeah, see my notes above -- I have an idea of how to do this
| } | ||
|
|
||
| /// Check if a StructArray has the expected Variant structure (metadata, value fields) | ||
| fn is_variant_struct_array(&self, struct_array: &StructArray) -> Result<bool, Box<dyn Error>> { |
There was a problem hiding this comment.
eventually, I think this could just reuse VariantArray which does all the same validations
However, to do that we would also need to add a new dependency on parquet-variant, and there is an interesting question about what crates depends on what
I think the idea is that we will NOT add any new dependencies to the main parquet crate. However, since this test harness is in that create, we can't add a dependency on VariantArray
So what I think we should do is as a follow on PR we can move this test into a new (testing only) crate like parquet-variant-integration-testing which can depend on both Variant and VariantArray
alamb
changed the title
# Which issue does this PR close? - Closes #8132 - Part of #8084 - Follow on to #8104 # Rationale for this change TLDR is we need a way to test and work out how Variant integration with the actual parquet reader/writer will look, so let's do it in the parquet crate. Please see the essay on #8132 for background Follow on tasks (I will file tickets for these items if we agree on this as an integration mechanism): - [x] Do not `panic` when writing VariantArray with the ArrowWriter: #8296 - [ ] Add some way to write the logical annotation to parquet metadata - [ ] Read arrays annotated with VARIANT logical type as VariantArrays in ArrowReader - [x] Update the variant_integration test to use `VariantArray` : #8084 - [x] Rename `variant_experimental` flag to `variant` and remove warnings about being experimental: #8297 Follow up tasks that came out of this PR but do not depend on it - [x] #8145 - [x] #8144 # What changes are included in this PR? 1. Add the `variant_experimental` feature to the `parquet` crate 2. Publicly export the variant crates 3. Add docs and examples # Are these changes tested? Yes by new CI # Are there any user-facing changes? This adds a new feature flag, and new --------- Co-authored-by: Matthijs Brobbel <[email protected]>
…ing` data (#8325) # Which issue does this PR close? We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. - Closes #8084 # Rationale for this change Now that we have merged the upstream parquet-variant tests: - apache/parquet-testing#91 We can test how far we are from the rust variant implementation working for all the values This PR updates the test harness added #8104 by @carpecodeum to use the final parquet files and the currnet APIs # What changes are included in this PR? 1. Update parquet-testing pin 2. Update the test harness to use the standard rust test runner (`#[test]`) rather than a custom main function 3. Added links to follow on tickets You can run this test manually like this: ```shell cargo test --all-features --test variant_integration ... running 138 tests test test_variant_integration_case_106 ... ok test test_variant_integration_case_107 ... ok test test_variant_integration_case_109 ... ok test test_variant_integration_case_110 ... ok .. test test_variant_integration_case_90 ... ok test test_variant_integration_case_91 ... ok test test_variant_integration_case_93 ... ok test test_variant_integration_case_83 - should panic ... ok test test_variant_integration_case_84 - should panic ... ok ``` # Are these changes tested? Yes this is all tests # Are there any user-facing changes? No
2 participants
Which issue does this PR close?
Rationale for this change
This PR implements comprehensive integration tests for Parquet files with Variant columns, using the real test data from parquet-testing PR #90.
Are these changes tested?
Yes
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
Are there any user-facing changes?
No
Thanks to @mprammer