-
Notifications
You must be signed in to change notification settings - Fork 1k
Update variant_integration
test to use final approved parquet-testing
data
#8325
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
variant_integration
to use final series
/// 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I needed to expose this information because the variant metadata / data are appended in one .bin file in the test cases
|
||
/// Test case definition structure matching the format from cases.json | ||
#[derive(Debug, Clone)] | ||
// Generate test functions for each case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
// - 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("); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am quite pleased how many cases pass, actually
variant_integration
to use final seriesvariant_integration
to use final approved parquet-testing
data
@carpecodeum or @mprammer do you have time to review this PR? |
Also FYI @scovich @codephage2020 @liamzwbao and @klion26 |
@scovich one thing I have been thinking about is if there is some way to leverage this same test suite for At the moment the tests read a shredded variant out as an unshredded varant and compares it I was thinking maybe I could extend this more to then re-shred the variant and compare it with the original shredded one 🤔 |
variant_integration
to use final approved parquet-testing
datavariant_integration
test to use final approved parquet-testing
data
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks for the review @klion26 I'll plan to merge this tomorrow unless anyone else would like additional time to review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good To Me! Thanks for the clean implementation.
The test is running more and more cases before I can even merge it! |
😅 -- testing for the win! |
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.
Rationale for this change
Now that we have merged the upstream parquet-variant tests:
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?
#[test]
) rather than a custom main functionYou can run this test manually like this:
Are these changes tested?
Yes this is all tests
Are there any user-facing changes?
No