Support adding fields to nested Parquet structs

[billonahill]

Presto currently support adding columns to a table, but not adding fields to a nested struct. This adds support for struct field additions.

[zhenxiao]

@billonahill ,
@jxiang has a PR, also resolving adding/deleting/reordering fields in Parquet structs:
#4714

[dain]

This check is used for all file formats, but it looks like only the Parquet record reader supports this. What will happen with the other formats?

[billonahill]

Do we want to move forward with this PR or PR #4714 (or both)? If we want to merge this one, which could be an iteration towards a the larger #4714, then I can add integration tests to verify the other formats.

[jxiang]

I think it is better to have PR #4714 at first. If needed, then we can get this one.

[nezihyigitbasi]

@billonahill @zhenxiao just to confirm with you guys, does #4714 supersede this PR? If yes please close this one and then we can focus on the other.

[billonahill]

@nezihyigitbasi I've confirmed that this issue still exists on master by re-submitting this PR with only the test changes included. I will check against #4714 as well. Is there a way to exercise the AbstractTestHiveClient.testTypesParquet test without using travis CI or running docker locally? I'd like to be able to just run the unit test locally but assertGetRecordsOptional doesn't execute.

[electrum]

@billonahill The purpose of that test is to read data that was written by Hive. You can create the table in Hive, then run TestHiveClient with the parameters set to point at your Hive installation. The test table is created by running create-test.sql and create-test-hive13.sql in presto-hive/src/test/sql in Hive. Though for just that one test, you can modify the scripts to remove anything unnecessary (can take a long time if your Hive cluster is slow).

[electrum]

For IntelliJ, you set the parameters in the "Parameters" tab of the TestNG run configuration. I run those Hive tests all the time in the IDE.

[billonahill]

Thanks @electrum for jogging my memory re how to run these tests, it's been a while. :) Adding my own notes here, this is how I run from the command line:

mvn -pl presto-hive-hadoop2 -P test-hive-hadoop2 -Dhive.hadoop2.databaseName=default -Dtest=TestHiveClient -Dmaven.surefire.debug test

@nezihyigitbasi #4714 does not seem to address this issue. What's needed is for HiveCoercionPolicy to handle structs, as well as a StructCoercionPolicy in HiveCoercionRecordCursor. One issue is that #4714 is pretty old and a lot has changed on master regarding coercion since it was implemented. We could merge #4714 and I can follow up with a patch, or we can include the test changes from this patch in #4714 and implement a fix. Or I can recreate this patch off master and we can commit it first. Let me know what you and @jxiang prefer.

For my own notes, I've made a branch based on the patch in #4714 (with the current master merged in) at https://github.com/billonahill/presto/tree/jxiang_extra_parquet_struct_fields which includes the fix to HiveCoercionPolicy, but still needs a fix to HiveCoercionRecordCursor.

[billonahill]

I've re-based the patch from master and updated it. This patch is ready to merge and can go in before #4714 (or #6675, which is much more extensive and with many merge conflicts).

[zhenxiao]

@billonahill not so clear about this PR, could you please add more explanation. Is there anything missing in #4714 for schema evolution in Parquet? We just use that and are working fine

[billonahill]

@zhenxiao if you include the unit tests from this patch only in #4714 you can see that they fail. I didn't investigate the code in detail to see why though. The patch in #4714 is based on a pretty old version of the master and has conflicts so I might not have resolved correctly based on your intent. If you could rebase it and try running with the tests from this patch that would show if it works or not.

[nezihyigitbasi]

@zhenxiao can you give @billonahill 's suggestion a try and let us know if that test works or not?

[zhenxiao]

@nezihyigitbasi @billonahill I rebased our Nested Schema Evolution work at:
#6675

It becomes an independent PR for Nested Schema Evolution. I will close all other pending PRs for that.

@billonahill could you please take a test on 6675 to see whether that could fix the problem? We've using that in production for a long time, to resolve all schema evolution issues. It is quite stable.

[billonahill]

I've commented on #6675 that the problem this patch addresses still exists, even after applying the branch in #6675. We could either merge #6675 and then pursue this issue, or vice-versa. Or we could incorporate the tests from this patch into #6675 and expand it's scope.

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!