alamb commented on code in PR #7738:
URL: https://github.com/apache/arrow-rs/pull/7738#discussion_r2162586376
##########
parquet-variant/src/variant.rs:
##########
@@ -850,28 +970,28 @@ impl From<i64> for Variant<'_, '_> {
impl From<(i32, u8)> for Variant<'_, '_> {
fn from(value: (i32, u8)) -> Self {
- Variant::Decimal4 {
+ Variant::Decimal4(VariantDecimal4 {
Review Comment:
I recommend changing this to `impl From<VariantDecimal4>`, otherwise this
path allows people to skip validation when creating variants
Similarly for the other 8 and 16 byte variants
##########
parquet-variant/src/variant.rs:
##########
@@ -40,8 +40,128 @@ const MAX_SHORT_STRING_BYTES: usize = 0x3F;
#[derive(Debug, Clone, Copy, PartialEq)]
pub struct ShortString<'a>(pub(crate) &'a str);
+#[derive(Debug, Clone, Copy, PartialEq)]
+pub struct VariantDecimal4 {
Review Comment:
Can you please add some comments here that explain waht this struct is, and
add a link to the spec that describes what the valid precision/scale values
are?
Similarly we should do the same thing for `8` and `16` byte variants too
##########
parquet-variant/tests/variant_interop.rs:
##########
@@ -63,9 +65,10 @@ fn get_primitive_cases() -> Vec<(&'static str,
Variant<'static, 'static>)> {
("primitive_boolean_false", Variant::BooleanFalse),
("primitive_boolean_true", Variant::BooleanTrue),
("primitive_date", Variant::Date(NaiveDate::from_ymd_opt(2025, 4 ,
16).unwrap())),
- ("primitive_decimal4", Variant::Decimal4{integer: 1234, scale: 2}),
- ("primitive_decimal8", Variant::Decimal8{integer: 1234567890, scale:
2}),
- ("primitive_decimal16", Variant::Decimal16{integer:
1234567891234567890, scale: 2}),
+ ("primitive_decimal4",
Variant::Decimal4(VariantDecimal4::try_new(1234, 2, 4).unwrap())),
Review Comment:
If you change the Variant::from impl above, this code would look like
```suggestion
("primitive_decimal4",
Variant::from(VariantDecimal4::try_new(1234i32, 2u8, 10).unwrap())),
```
If you make the `TryFrom` impl, it would look like
```suggestion
("primitive_decimal4", Variant::try_from(1234, 2, 4).unwrap())),
```
##########
parquet-variant/src/variant.rs:
##########
@@ -850,28 +970,28 @@ impl From<i64> for Variant<'_, '_> {
impl From<(i32, u8)> for Variant<'_, '_> {
fn from(value: (i32, u8)) -> Self {
- Variant::Decimal4 {
+ Variant::Decimal4(VariantDecimal4 {
Review Comment:
If we want to keep these (i32, u8) constrsuctors, I think we should
implement `TryFrom<(i32, u8)>` so we can properly verify the errors
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
