Improve ergonomics of parquet::basic::LogicalType

[tustvold]

Is your feature request related to a problem or challenge? Please describe what you are trying to do.

LogicalType is defined as

pub enum LogicalType {
    STRING(StringType),
    MAP(MapType),
    LIST(ListType),
    ENUM(EnumType),
    DECIMAL(DecimalType),
    DATE(DateType),
    TIME(TimeType),
    TIMESTAMP(TimestampType),
    INTEGER(IntType),
    UNKNOWN(NullType),
    JSON(JsonType),
    BSON(BsonType),
    UUID(UUIDType),
}

Where the inner structs are actually defined by parquet_format. This not only requires an additional crate in scope, but also a number of these are actually empty structs resulting in constructions like

LogicalType::String(Default::default)

Describe the solution you'd like

I would like to change the signature to be instead

pub enum LogicalType {
    STRING,
    MAP,
    LIST,
    ENUM,
    DECIMAL {
        scale: i32,
        precision: i32,
    },
    DATE,
    TIME {
        is_adjusted_to_u_t_c: bool,
        unit: TimeUnit,
    },
    TIMESTAMP {
        is_adjusted_to_u_t_c: bool,
        unit: TimeUnit,
    },
    INTEGER {
        bit_width: i8,
        is_signed: bool,
    },
    UNKNOWN,
    JSON,
    BSON,
    UUID,
}

I would also like it to implement Copy

Describe alternatives you've considered

We could leave the enumeration unchanged

[alamb]

cc @nevi-me and @sunchao

[tustvold]

We could also change the variants to be upper camel case, as is more idiomatic in Rust code 🤔

[sunchao]

+1. I think the new proposal looks cleaner. Also +1 on changing to CamelCase.

[tfeda]

Hi all, I've started implementing this. Would we want the string parser to also use CamelCase?
for example:

let message = " 
message test_schema {
    REQUIRED INT32   uint8 (INTEGER(8,false));
    REQUIRED INT32   uint16 (INTEGER(16,false));
} 
"

would be changed to

let message = " 
message test_schema {
    REQUIRED INT32   uint8 (Integer(8,false));
    REQUIRED INT32   uint16 (Integer(16,false));
} 
"

I originally came across this when starting to edit error messages, and wasn't sure if printing the enum's format or the string's format is more instructive to the user.

Ex:
general_err!("Failed to parse timeunit for Timestamp type")

[tustvold]

The convention for expressing parquet schema appears to be SHOUTY CASE, I would therefore avoid making changes to the parser to not accept this. I suppose you might be able to make it case-insensitive, but I'm not sure if this is spec-compliant, we'd need to check 🤔

As for error messages, I think whatever is easiest. I personally prefer camel case as I don't like my error messages screaming at me, but the important thing is the meaning is clear 👍