(parquet-avro) Automatically detect list encodings in AvroReadSupport

[clairemcginty]

Rationale for this change

parquet-avro supports writing both "old" and "new" list encodings via the parquet.avro.write-old-list-structure config. "old" encodings (aka "2-level"), which wrap the list in a repeated group array schema, are the default; "new" encodings (aka "3-level") are opt-in.

On the reader side, if you're using ParquetAvroReader to read data that was written using ParquetAvroWriter, and don't specify a projection, both type sof list encoding get parsed automatically from a combination of the file schema + the parquet.avro.schema metadata key. There's no need to set parquet.avro.write-old-list-structure key in your Configuration.

However, if you're either:

• specifying a projection (AvroReadSupport.setRequestedProjection(...)), or
• reading data not written using ParquetAvroWriter (and thus not containing the parquet.avro.schema metadata key),

3-levle list encodings will not be parsed correctly - the reader will inject an extra nested record, named element, into the list item type.

As a reader this introduces some pain, since you have to look up the underlying file metadata of the upstream Parquet file, and risk reading incorrect data. This PR attempts to automatically detect new list encodings based on the writer file schema.

lmk what you think of this change. Automatic inference is always a bit risky, but I tried to be conservative with the approach (only set the list structure property if all list fields in the schema use 3-level encoding; don't override parquet.avro.write-old-list-structure if the user is already setting it). any ideas for a better approach here are welcome - this is becoming more of a pain point as 3-level lists become a more popular option among other writer sdks.

What changes are included in this PR?

A new read configuration property parquet.avro.read.autoDetectListStructure (defaulting to true) that will instruct AvroReadSupport to automatically set List configuration properties based on parsing the writer file schema.

Are these changes tested?

Yes, unit tests + locally on real data.

Are there any user-facing changes?

Yes, since the new property defaults to true - it would impact anyone who's reading 3-level list data without setting the parquet.avro.write-old-list-structure key and who's relying on/working around the incorrectly formatted data (e.g. {"locations": [{"element": {"latitude": 0.0, "longitude": 180.0}}, ...]} instead of {"locations": [{"latitude": 0.0, "longitude": 180.0}, ...]} .

additionally, this change also modifies the underlying Configuration object to add the properties.

[wgtmac]

Is it a good time to make a shift on the default value of AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE? It has caused a lot of troubles.

[clairemcginty]

it would be nice to have the more modern encoding as default! though I'm a little nervous about the implications of changing the schema for existing datasets - I know we have some downstream use cases where a single Parquet reader is reading a globbed filepattern matching multiple partitions of the same dataset - not sure what would happen if some partitions used 2-level encoding and some used 3-level encoding. Same concern about Parquet datasets that are the source of truth for external tables in Snowflake/BigQuery/etc.

maybe I can create an issue for this and request a bit more investigation on possible implications of this change?

[wgtmac]

Yes, it should be a separate concern. Let's not mix it in the current PR.

[wgtmac]

This writes the inferred mode back into the shared Configuration. ParquetReader reuses that configuration across files, so after a 3-level file, a later 2-level file (especially with a projection) is still converted as 3-level. Could this stay per-file/read-context instead of mutating the caller's configuration?

[clairemcginty]

Yeah - I was a bit worried about that. Mutating the caller conf is simplest because we want to have these properties set in both init() (when computing the projection) and prepareForRead() (when computing avroSchema if it's not already set in file footer/read conf). What do you think about modifying metadata instead of conf? like:

public ReadContext init(
      ParquetConfiguration configuration, Map<String, String> keyValueMetaData, MessageType fileSchema) {
    MessageType projection = fileSchema;
    Map<String, String> metadata = new LinkedHashMap<String, String>();

    boolean autoDetectListStructure =
        configuration.getBoolean(AUTO_DETECT_LIST_STRUCTURE, AUTO_DETECT_LIST_STRUCTURE_DEFAULT);

    if (autoDetectListStructure
        && configuration.get(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE) == null
        && configuration.get(AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS) == null) {
      if (writesNewListStructure(fileSchema)) {
-        configuration.setBoolean(AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE, false);
-        configuration.setBoolean(AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS, false);
+        metadata.put("inferred." + AvroWriteSupport.WRITE_OLD_LIST_STRUCTURE, "false");
+        metadata.put("inferred." + AvroSchemaConverter.ADD_LIST_ELEMENT_RECORDS, "false");
      }
    }

+ // ...read those properties in prepareForRead() and apply to copied Configuration

Or - we could just re-compute writesNewListStructure(fileSchema) in both init() and prepareForRead(). that might be more straightforward overall 🤷‍♀️

[wgtmac]

I may be misremembering the LIST compatibility rules, so I wanted to check this edge case: could a legacy 2-level list also use the list/element names and therefore look like this to allListStructuresAreThreeLevel? If so, would auto-detect change its Avro shape unexpectedly, or is this case ruled out by the spec or writer assumptions?

[clairemcginty]

added a few more test cases here and the auto-detect correctly returns false for those cases! lmk if you had any other cases in mind...