[Parquet] reader appears to have bug when supporting unknown sort orders

[emkornfield]

Describe the bug

The current reader code for interpreting sort orders, does not appear to push back any elements into the Vec if the sort order does not match TYPEORDER. If this is correct reading (not very familiar with Rust). Then for any new sort orders added there is likely going to be a panic or error when trying to make use of the returned vector.

To Reproduce

N/A, this is from code inspection.

Expected behavior

When an unmatched sort order is encountered UNKNOWN/Undefined sort order is pushed back

Additional context

I was reading this code in the context of how to handle recent discussions around defining int96 sort order in the specification.

[etseidl]

IIRC, this is actually a limitation of the rust thrift generator. An undefined union field will panic. It used to have a similar problem with undefined enum values, but that was fixed (see https://issues.apache.org/jira/browse/THRIFT-5314?page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel&focusedCommentId=17235158).

I was struggling with this while trying to implement a new sort order for floats (#7408). Files written with the new sort order would panic while trying to parse the thrift.

I guess we could bring this up with the thrift devs, but also adds ammunition to writing our own thrift parsing (this was brought up in https://www.influxdata.com/blog/how-good-parquet-wide-tables/)

cc @alamb @XiangpengHao

[XiangpengHao]

Like see our own thrift parsing, wonder if it's much easier with llm...

[alamb]

I would also love to pour ⛽ on the 🔥 of parquet footer design with a custom thrift parser

However, I am not likely to have the time to help with such a project for a while

[jhorstmann]

The current reader code for interpreting sort orders, does not appear to push back any elements into the Vec if the sort order does not match TYPEORDER.

I think the generated thrift parser would already have returned an error here for an unknown ColumnOrder. I don't think it should panic though.

What behavior would we want from a thrift parser? To avoid this problem, each generated enum would need to have some UNKNOWN / UNSUPPORTED variant. For ColumnOrder we could just ignore the statistics in that case, but I'm not sure whether in general this would just move the error to another place in the code.

[etseidl]

What behavior would we want from a thrift parser? To avoid this problem, each generated enum would need to have some UNKNOWN / UNSUPPORTED variant.

As I understand it, thrift is meant to be forward compatible, and ColumnOrder is definitely intended to be so. In C++, unions are modeled as a struct of structs along with a bitfield indicating which, if any, of the members are valid. If an unknown value is encountered, it simply returns an empty struct with all bit fields '0'.

For enum, the rust thrift implementation was changed from a rust enum to a struct with a single i32 for the discriminant, with constants defined for each known value. An unknown value i is returned as EnumName(i), which removes the need for an UNDEFINED special value.

We could take a similar approach for union. A struct with an "enum struct" discriminant, along with Option<struct> fields for each variant. An unknown i would then have i for the enum part and None for all members. Getters for the fields could test the discriminant and return None if it is incorrect for the field.

For ColumnOrder we could just ignore the statistics in that case, but I'm not sure whether in general this would just move the error to another place in the code.

In the specific ColumnOrder case, its as-designed behavior is that when an unknown order is encountered, statistics for that column are to be ignored. This is not currently enforced in rust, but @adriangb and I attempted to address it in apache/datafusion#15821. arrow-rs should likely also be changed to not populate statistics for columns with undefined orders.

[etseidl]

I think the generated thrift parser would already have returned an error here for an unknown ColumnOrder. I don't think it should panic though.

That's my bad...going off memory. The parsing code doesn't panic, it just returns an error. parquet-read panics when it encounters the error. But even erroring in this case is problematic because it doesn't allow ColumnOrder to be forward compatible as intended.

[emkornfield]

For ColumnOrder we could just ignore the statistics in that case, but I'm not sure whether in general this would just move the error to another place in the code.

I'd argue the specific intent for ColumnOrder being unknown means statistics should be ignored.

but I'm not sure whether in general this would just move the error to another place in the code.

It really is likely a case by case issue (i.e. at the parquet level) not the at the thrift level on whether an unknown enum should fail the read. But in most cases at least, one wouldn't want to necessarily fail at thrift parsing time because of the different type of filtering that parquet supports post-footer reading.

[etseidl]

FWIW I put up #7930 as an illustration of what I'm talking about above.

[jhorstmann]

What I had in mind on the thrift side would look like this:

pub enum ColumnOrder {
    TYPE_ORDER(TypeDefinedOrder),
    __UNKNOWN__ { field_id: i16 },
}

Multiple fields being set in a thrift union would still be a protocol-level error, so modeling this as a rust enum should work fine.

[etseidl]

Filed https://issues.apache.org/jira/browse/THRIFT-5890.

I agree with @jhorstmann that using an enum with an "unknown" field is more ergonomic and rusty...I'm still trying to figure out why the struct approach was chosen for enums. There's a discussion here to switch back to enums, but it seems to have died. If we wind up rolling our own, I'd opt for enums.

@jhorstmann should we take another crack at your macro based thrift decoder?

[etseidl]

Just submitted a draft that illustrates the enum approach. #7995