GH-465: Clarify backward-compatibility rules on LIST type

[wgtmac]

Rationale for this change

The C++ reader of parquet-cpp is having a hard time to read Parquet file written by parquet-java with parquet.avro.write-old-list-structure=true and schema below:

optional group a (LIST) {
  repeated group array (LIST) {
    repeated int32 array;
  }
}

See apache/arrow#43994 and apache/arrow#43995

What changes are included in this PR?

Clarify the rules for various list types.

1. Unannotated repeated fields.

repeated int32 num_list;

repeated group my_list {
  required int32 num;
  optional binary str (STRING);
}

2. LIST-annotated 2-level structures.

<list-repetition> group <name> (LIST) {
  repeated <element-type> <element-name>;
}

list-repetition: can be repeated, required, and optional.

Do these changes have PoC implementations?

Not required

Closes #465

[mapleFU]

BTW, can you also point out the Java code in this pr?

[mapleFU]

Other LGTM but I think it worths issue a disscussion...

[wgtmac]

I have sent a discussion thread to the dev ML. It would be good if you can take a look. Thanks! @emkornfield @pitrou @gszadovszky @rdblue @etseidl @clairemcginty

[etseidl]

Seems like a nice clarification to me. One little (ignorable) nit.

I'm also wondering if we should add an example above for the non-LIST annotated repeating fields, or should that be a new PR?

[mapleFU]

Thanks for clarifying! This is so tricky especially rule 3 and rule 4 in 2-level lists...

[etseidl]

Thanks for being patient with me Gang 😄. And thanks for taking on this needed clarification.

[etseidl]

No, the middle level can never be a LIST-annotated or MAP-annotated 3-level structure, because they cannot be repeated.

Ah, ok. Now I understand. That's why the 2-level list needs the ability to be repeated, but only as part of a nested list. Thanks again for the clarification @wgtmac. I think all my questions are answered now.

[wgtmac]

@gszadovszky @etseidl @rdblue @pitrou @emkornfield Do you have any concern about this PR? I think we need to move forward now.

[etseidl]

All my concerns have been addressed. Thanks!

[mapleFU]

Rest LGTM

[rdblue]

I think this example is counter-productive. We don't want anyone using un-annotated lists and maps. While the paragraph above explains how to interpret un-annotated repeated fields, I don't want anyone to see an example here and think that it is something that should be copied. I think it is already clear enough and I would simply move on rather than drawing attention to this as a possibility.

[wgtmac]

That make sense. Let me remove these examples first. I think a followup is to deprecate it by moving it to the backward compatibility section and adding strong words to discourage writers to emit it.

[rdblue]

The rules part is looking good, but I think that spending time documenting what people did incorrectly years ago makes the doc more confusing and increases chances that people will write invalid lists. I'd prefer to revert most of the changes that explain what people did incorrectly.

[mapleFU]

The rules part is looking good, but I think that spending time documenting what people did incorrectly years ago makes the doc more confusing and increases chances that people will write invalid lists. I'd prefer to revert most of the changes that explain what people did incorrectly.

I agree. But I think those can be posted on the pull-request description

[wgtmac]

@rdblue Thanks for your review! I have removed all unnecessary changes. Please take a look again.

[pitrou]

@rdblue @wgtmac I'm not sure I agree with removing the examples. They make it less likely that an implementation would interpret the written rules wrongly, IMO. Maybe we can instead strengthen the wording that these examples show existing legacy schemas, not anything to be written by modern writers.

[pitrou]

Concretely, if I have to review a PR implementing said compatibility rules, I'd rather have the spec spell out the examples because it makes it easier to check the PR's implementation (and unit tests!).

[wgtmac]

Perhaps add them like this? @pitrou

WARNING: WRITERS SHOULD NOT PRODUCE LIST TYPES LIKE THESE EXAMPLES!
THEY ARE FOR THE PURPOSE OF READING EXISTING DATA ONLY.

// List<Integer> (non-null list, non-null elements)
repeated int32 num;

// List<Tuple<Integer, String>> (non-null list, non-null elements)
repeated group my_list {
  required int32 num;
  optional binary str (STRING);
}

[pitrou]

Ok, please don't use UPPERCASE :)

[wgtmac]

@pitrou @gszadovszky @rdblue Do you have any concern with the latest change?

[gszadovszky]

I do not have any concerns. Looks good to me!

[wgtmac]

Will merge this by the end of this week if no objection.