Refactor parquet thrift reader

[etseidl]

Description

Refactors the current CompactProtocolReader used to parse parquet file metadata. The main goal of the refactor is to allow easier use of std::optional fields in the thrift structs to prevent situations as in #14024 where an optional field is an empty string. The writer cannot distinguish between present-but-empty and not-present, so chooses the latter when writing the field. This PR adds a ParquetFieldOptional functor that can wrap the other field functors, obviating the need to write a new optional functor for each type.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[etseidl]

I've also moved the definitions of the field functors to compact_protocol_reader.cpp as they are not required to be in the header for CPR to function properly. Having them defined in the header also leads to unnecessary compilations when fields or structs are modified.

Also added an implementation PoC for unions that uses optional fields rather than an isset struct.

[vuule]

quick first pass
thanks for clearing out the header!

[vuule]

Is this equivalent to bool is_type_defined_order?

[etseidl]

The parquet spec says unions are like enhanced enums. Many of the fields are just placeholders to signify which field is valid, but then the fields can have state, as with LogicalType. Right now, LogicalType is a struct of structs, with a struct field that's a struct of bools to indicate which field is valid. I'm proposing replacing that setup with a struct of optional structs, where only one field has a value.

ColumnOrder is a strange case because there's only one defined value at present, but that may change in a bit.

[etseidl]

Thinking more about this, since this is supposed to be thought of as an enum, we could do something like:

struct TypeDefinedOrder {};

struct SomethingElse {
  int some_state;
};

struct ColumnOrder {
  enum class Type {
    UNDEFINED = 0,
    TYPE_ORDER = 1,
    SOMETHING_ELSE = 2
  };
  std::optional<TypeDefinedOrder> type_order;
  std::optional<SomethingElse> something_else;

  Type operator() const {
    if (type_order.has_value()) { return Type::TYPE_ORDER; }
    if (something_else.has_value()) { return Type::SOMETHING_ELSE; }
    return Type::UNDEFINED;
  }

  // or
  Type type; // assigned during read
  Type operator() const { return type; }
};

Then the ColumnOrder could be used in a switch statement, and the case for SOMETHING_ELSE could access column_order.something_else.value().some_state. Empty structs could be replaced with a bool I suppose, or not even be defined if we save the enumerator value (the logic for empty struct is already separate from the non-empty case).

I would probably do it this way if I was writing a generic thrift parser, but for just parsing the parquet types, I think this may be overkill 😉

[etseidl]

Wound up making the above change (with mods to make it compile). I think treating these unions as enums is the way to go. And having a bunch of empty optional structs is unnecessary and takes up space.

[PointKernel]

Looks great.

Depending on how @vuule sees it, shall we take this chance to rename involved classes or functions with snake_case to align with libcudf guidance?

[vuule]

Looks great.

Depending on how @vuule sees it, shall we take this chance to rename involved classes or functions with snake_case to align with libcudf guidance?

I like to keep such changes separate.
@etseidl do we have naming directly from PQ specs in the code here? For example, we have a bunch of types in ORC that keep the ORC specs names.

[etseidl]

do we have naming directly from PQ specs in the code here?

For the most part, yes. So from the parquet thrift IDL:

/** Empty struct to signal the order defined by the physical or logical type */
struct TypeDefinedOrder {}

union ColumnOrder {
  1: TypeDefinedOrder TYPE_ORDER;
}

and I'm adding

struct ColumnOrder {
  enum Type { UNDEFINED, TYPE_ORDER };
  Type type;

  operator Type() const { return type; }
};

There are a few places where the names don't quite match, so ColumnMetaData from the thrift becomes ColumnChunkMetaData in cudf. The parquet spec uses CamelCase for type names, and snake_case for field names.

But for function names we're certainly free to switch to snake_case as @PointKernel suggests.

[etseidl]

So, assuming we're keeping the names from parquet as they are, shall I rename CompactProtocolReader, its functions, and all the field functors in compact_protocol_reader.cpp? Or just the functors that don't change the interface, and save the more drastic changes for another day?

[vuule]

few nitpicks, looks good otherwise

[vuule]

/ok to test

[PointKernel]

Looks great! Thanks for the refactoring.

shall I rename CompactProtocolReader, its functions, and all the field functors in compact_protocol_reader.cpp?

We could do it in a separate PR and clean up both thrift reader and writer.

[PointKernel]

/merge