avro.schema.literal support and some related patches

[lxynov]

Closes #5001
A related issue: prestodb/presto#9116

[jiegzhan]

@lxynov, you have anything to add to this PR? Thanks.

[lxynov]

@findepi Could you help review this? Some commits seems hacky (due to Hive's lack of spec and doc). Feel free to slack me if you'd like to discuss in DMs.

[jiegzhan]

@lxynov @findepi Hey guys, any progress on this PR? Thanks.

[findepi]

Can you please add a test (product test?) for partitioned tables where table and partition schema mismatches?

for example

• table is ORC, partition is Parquet
• table is ORC, partition is Avro with schema url
• table is ORC, partition is Avro with schema literal
• table is ORC, partition is CSV

[lxynov]

I was trying to add such a product test (using HQLs) which fails before the commit and succeeds after the commit, but failed to do so. But I think this commit logically makes sense and it has been deployed at our company for a long time. Please let me know if you have concerns.

[findepi]

Can there be any other exception flying here?

[lxynov]

By looking at the implementation of Schema.Parser::parse(), I think SchemaParseException is sufficient here

[findepi]

I think the reason not to support CTAS with schema url is because we need the table to be in metastore in order to get the information what the schema actually (at least this is the current implementation).

For schema literal we could obtain this information locally. Even if we do not do this (which I am fine with), we should keep those cases separate here -- as separate ifs.

[lxynov]

Separated ifs

[findepi]

avro.schema.literal support and some related patches

the table/partition schema handliung changes require some improvement
would it be possible to separate avro.schema.literal into its own PR, or are there too many dependencies?

[lxynov]

Hi @ebyhr @findepi , this PR is ready for another review. Please let me know if you have questions around any of the commits.

[ebyhr]

Could you add test cases about schema evolution (maybe in TestAvroSchemaEvolution)? This PR is going to close #5001, but the test looks not covered.

[ebyhr]

It would be better to limit onHive() usage for Hive specific operation because it's slow. Other places are also the same.

[lxynov]

This test is meant to test Hive created tables, so onHive() is used here.

[lxynov]

@ebyhr Thanks for the review! Updated.

Could you add test cases about schema evolution (maybe in TestAvroSchemaEvolution)? This PR is going to close #5001, but the test looks not covered.

Added in TestAvroSchemaEvolution

[lxynov]

@ebyhr All tests have passed. This is ready for review

[lxynov]

@ebyhr @findepi Could you please take a look? Thanks in advance!

would it be possible to separate avro.schema.literal into its own PR, or are there too many dependencies?

@findepi I thought about this before but found that commit Allow creating tables with Avro schema literal sort of depends on commit Call getFields for serdes not using use metastore for schema, as the later commit changed method isAvroTableWithSchemaSet to isTableSerdesUsingMetastoreForSchema

[colebow]

👋 @lxynov - this PR has become inactive. If you're still interested in working on it, please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.