enforce non empty group on MessageType level

[asfimport]

As columnar format, parquet currently does not support empty struct/group without leaves. We should throw when constructing an empty GroupType to give a clear message.

Reporter: Tim / @tsdeng
Assignee: Tim / @tsdeng

Related issues:

• A bad performance regression issue with Parquet happens if Hive does not select any columns (relates to)
• Cannot construct empty MessageType for ReadContext.requestedSchema (is related to)

PRs and other links:

• GitHub Pull Request #1111
• This issue is resolved by PR #195, authored by tsdeng

_{Note: This issue was originally created as PARQUET-278. Please see the migration documentation for further details.}

[asfimport]

Ryan Blue / @rdblue:
What about deprecating the Type constructors in favor of the builders? We already do this kind of check there: https://github.com/apache/parquet-mr/blob/master/parquet-column/src/main/java/org/apache/parquet/schema/Types.java#L554

[asfimport]

Tim / @tsdeng:
It's good to double check empty fields in the constructor of GroupType.
Even if we make the constructor non public, the fluent builder api still calls it.

Migrating to using the builder API could be in another separate PR.

Also not sure if

Types.buildMessage()
.required(INT64).named("DocId")
.optionalGroup()
.repeated(INT64).named("Backward")
.repeated(INT64).named("Forward")
.named("Links")
.repeatedGroup()
.repeatedGroup()
.required(BINARY).named("Code")
.optional(BINARY).named("Country")
.named("Language")
.optional(BINARY).named("Url")
.named("Name")
.named("Document");

really looks better than the constructor API in terms of readability since parquet schema is a nested structure, but the fluentAPI needs manually indentation to reflect the nested structure. The constructor based API is simple and clear, does its job nicely which is constructing objects and IDE can indent it pretty well.

new MessageType("Document",
new PrimitiveType(REQUIRED, INT64, "DocId"),
new GroupType(OPTIONAL, "Links",
new PrimitiveType(REPEATED, INT64, "Backward"),
new PrimitiveType(REPEATED, INT64, "Forward")
),
new GroupType(REPEATED, "Name",
new GroupType(REPEATED, "Language",
new PrimitiveType(REQUIRED, BINARY, "Code"),
new PrimitiveType(OPTIONAL, BINARY, "Country")),
new PrimitiveType(OPTIONAL, BINARY, "Url")));

[asfimport]

Tim / @tsdeng:
We could actually change the fluentAPI to be more clear about showing the nested structure. like:

message("myMessage").with(
group("firstGroup").with(
primitive("age").type("INT").optional()
)
)

Instead of building an API that chains everything together, I prefer an DSL that actually look s more like the real structured schema

[asfimport]

Ryan Blue / @rdblue:
@tsdeng, I'm fine changing the API a bit, but using the constructors doesn't help ensure types are valid. That's the primary reason for moving to the Types API, whatever it ends up looking like.

[asfimport]

Tim / @tsdeng:
Not sure what you mean by "using constructors doesn't help ensure types are valid". With the fluentAPI, you do the check in the builder, with constructors, you put the check in the constructor. I don't see in the case of constructing a MessageType, what can be done in the builder can not be done in the constructor.

The goal is to construct a tree(nested) MessageType object, IMO, the things that matters are:

1. Prefer API to reflect the structure of the schema being constructed
2. The simpler the better

No strong opinion on doing it on builders or constructors though, but prefer simplicity.

[asfimport]

Ryan Blue / @rdblue:
It's just more difficult to maintain validations in the constructors and we don't want to maintain them in two places. Plus, a builder can do a better job with the API over time. Constructors have to be deprecated and replaced.