Flexible parquet struct converter

[jxiang]

No description provided.

[jxiang]

Our parquet struct schema keeps evolving. Very frequently, we have parquet files of different versions co-existing. Instead of failing the query due to schema mismatching, it is better to return null for new fields if the data file is old.

[nezihyigitbasi]

Sometimes this check helps to catch problems with the data definition in the metastore, so I think silently returning nulls if the schemas do not match is not the right way to go.

[jxiang]

This check is good but too restrictive. It makes it hard to evolve the schema seamlessly. We return nulls and log some message below so that queries can still go on. If they pay attention to the logging, they should realize what's happened.

Even if the number matches, the schema still could be incompatible. Users still need to know what they are doing.

[nezihyigitbasi]

can you add unit tests?

[nezihyigitbasi]

I don't think we need the log here (especially in INFO level)

[jxiang]

It's good to log something since we don't fail the query any more. I can make this a warning.

[jxiang]

Sure, will add some unit tests.

[jxiang]

Added a unit test to the patch.

[jxiang]

Updated the patch: 1) now we support both adding and removing struct fields; 2) support adding fields at any place in the struct; 3) added more test cases for these scenarios.
However, it is not supported to change the order of existing fields in a struct. In such a case, a schema mismatch error will be thrown.

[jxiang]

Added another patch that supports changing the order of existing fields in a struct. With these patches, now we fully support Parquet struct schema evolution.

• The schema from the metastore is the source of truth
• Use field order from the metastore schema,
• Field in metastore schema but not in parquet schema, has null value,
• Filed not in metasotre schema but in parquet schema, is ignored.

Schema mismatch will be logged although query executes.

[zhenxiao]

Is it possible to update all BlockConverter into BlockFieldConverter? Then we just need on BlockConverter, with fieldIndex

[jxiang]

Most of converters can share the same interface, except ParquetListEntryConverter and ParquetMapEntryConverter. The field index doesn't apply to these two entry converters.

[dain]

@zhenxiao now that the new parquet reader supports structs, we should add this same feature to it

[zhenxiao]

@dain yes, we are working / stress testing it
New Parquet Reader is using nested path names to look up ColumnDescriptors, it has most schema evolution support now

[markcho]

@jxiang Is there anything that I can do to help out with this PR?

I'm facing the same problem where we have mismatching schemas for structs due to schema evolution and it's not very feasible for us backfill the old Parquet files to match the new schemas.

I can apply this change to my fork but I think other people may find this feature useful as well.

That being said, is there a different approach to schema evolution involving Parquet files for cases similar to this, without applying this patch?

[billonahill]

+1 to @markcho's comment. We're also in need of this patch as well. cc/ @Yaliang.

[Gauravshah]

@zhenxiao the code still doesn't allow for struct types to evolve https://github.com/prestodb/presto/blob/master/presto-hive/src/main/java/com/facebook/presto/hive/parquet/ParquetHiveRecordCursor.java#L743-L748

anything we can do to help in this pr ?

[zhenxiao]

we have a rebased version here:
ba5a3e4

[Gauravshah]

adding the updated pull request for reference #6675

[jxiang]

Thanks @zhenxiao, I pushed the rebased version to this branch.

[jxiang]

@dain could you take a look when you get a chance? Thanks.

[dain]

@jxiang yep. I see there are two (or three) PRs related to this. Can you help me understand which ones I should review in which order?

[zhenxiao]

@dain this PR is the very first one, now @jxiang has all schema evolution stuff in one commit. #6675 is built on top of this

[jxiang]

Yeah, as @zhenxiao said, this is the first one. Thanks.

[nezihyigitbasi]

@zhenxiao @jxiang AFAIU #6675 supersedes this one. If that's correct please close this PR and then we can work on the other one.

[zhenxiao]

continue with:
#6675