[Variant] Add negative tests for reading invalid primitive variant values #7779
Conversation
standardize naming
parquet-variant/Cargo.toml
Outdated
| [dependencies] | ||
| arrow-schema = { workspace = true } | ||
| chrono = { workspace = true } | ||
| paste = { version = "1.0" } |
There was a problem hiding this comment.
Used for writing the decoder test macros. This is already a dependency in the parquet crate, so bringing it into parquet-variant.
There was a problem hiding this comment.
If it is only used in tests, perhaps we can move it to dev-dependencies
[dev-dependencies]
paste = { version = "1.0" }
parquet-variant/src/decoder.rs
Outdated
| use super::*; | ||
| use paste::paste; | ||
|
|
||
| mod integer { |
There was a problem hiding this comment.
Created macros and grouped similar tests together in order to reduce some of the boilerplate test code.
parquet-variant/src/decoder.rs
Outdated
|
|
||
| // Makes the code a bit more readable | ||
| pub(crate) const VARIANT_VALUE_HEADER_BYTES: usize = 1; | ||
| pub(crate) const DECIMAL_MAX_SCALE: u8 = 38; |
There was a problem hiding this comment.
According to the spec, all the decimal variants have a max scale of 38.
There was a problem hiding this comment.
- I think this somewhat overlaps with @scovich 's PR here: [Variant] Add negative tests for reading invalid primitive variant values #7779
#7779 does not include any decoder tests though
I think we could potentially avoid checking the scale/precision for decimals in the decoder and let the higher level validation check them
There was a problem hiding this comment.
That makes sense. I removed the extra scale validation.
superserious-dev
changed the title
alamb
left a comment
alamb
left a comment
There was a problem hiding this comment.
Thank you @superserious-dev -- I think this is a very nice improvement. I have some suggestions on how to improve the testing, but I don't think they are required
I suspect there will be some conflicts to resolve after I merge #7776 which I am about to do
parquet-variant/Cargo.toml
Outdated
| [dependencies] | ||
| arrow-schema = { workspace = true } | ||
| chrono = { workspace = true } | ||
| paste = { version = "1.0" } |
There was a problem hiding this comment.
If it is only used in tests, perhaps we can move it to dev-dependencies
[dev-dependencies]
paste = { version = "1.0" }
parquet-variant/src/decoder.rs
Outdated
|
|
||
| // Makes the code a bit more readable | ||
| pub(crate) const VARIANT_VALUE_HEADER_BYTES: usize = 1; | ||
| pub(crate) const DECIMAL_MAX_SCALE: u8 = 38; |
There was a problem hiding this comment.
- I think this somewhat overlaps with @scovich 's PR here: [Variant] Add negative tests for reading invalid primitive variant values #7779
#7779 does not include any decoder tests though
I think we could potentially avoid checking the scale/precision for decimals in the decoder and let the higher level validation check them
|
Thanks again @superserious-dev |
2 participants
Which issue does this PR close?
Rationale for this change
Follow-up from the previous PR that added decoders for primitive values.
What changes are included in this PR?