avro.schema.literal support and some related patches #5101

lxynov · 2020-09-08T15:29:45Z

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

jiegzhan · 2020-09-18T16:50:08Z

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

lxynov · 2020-09-23T21:33:59Z

@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 · 2020-10-13T18:47:39Z

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

findepi · 2020-10-16T11:09:13Z

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/thrift/BridgingHiveMetastore.java

-        if (isAvroTableWithSchemaSet(table)) {
+        // If a partition's Serde doesn't use metastore for schema (like Avro), we use table-level schema. Data publishers
+        // are responsible for making sure that table-level schema is workable for all partitions.
+        if (!isPartitionSerdesUsingMetastoreForSchema(partition)) {


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

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.

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

findepi · 2020-10-16T11:40:26Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

+            new Schema.Parser().parse(avroSchemaLiteral);
+            return avroSchemaLiteral;
+        }
+        catch (SchemaParseException e) {


Can there be any other exception flying here?

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

findepi · 2020-10-16T11:42:44Z

presto-hive/src/main/java/io/prestosql/plugin/hive/HiveMetadata.java

-        if (getAvroSchemaUrl(tableMetadata.getProperties()) != null) {
-            throw new PrestoException(NOT_SUPPORTED, "CREATE TABLE AS not supported when Avro schema url is set");
+        if (getAvroSchemaUrl(tableMetadata.getProperties()) != null || getAvroSchemaLiteral(tableMetadata.getProperties()) != null) {
+            throw new PrestoException(NOT_SUPPORTED, "CREATE TABLE AS not supported when Avro schema url or literal is set");


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.

Separated ifs

findepi · 2020-10-16T11:44:36Z

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 · 2020-12-17T20:41:00Z

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 · 2021-01-01T09:40:24Z

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestAvroSchemaLiteral.java

+    @Test(groups = AVRO)
+    public void testHiveCreatedTable()
+    {
+        onHive().executeQuery("DROP TABLE IF EXISTS test_avro_schema_literal_hive");


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

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

presto-product-tests/src/main/java/io/prestosql/tests/hive/TestAvroSchemaLiteral.java

presto-hive/src/main/java/io/prestosql/plugin/hive/metastore/MetastoreUtil.java

lxynov · 2021-01-07T04:43:12Z

@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

…e for schema

lxynov · 2021-01-07T22:52:03Z

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

lxynov · 2021-01-19T17:40:29Z

@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 · 2022-10-19T21:41:59Z

👋 @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.

lxynov marked this pull request as draft September 8, 2020 15:29

lxynov force-pushed the avro branch from 60e1b8b to 757642b Compare September 22, 2020 05:54

cla-bot bot added the cla-signed label Sep 22, 2020

lxynov force-pushed the avro branch 2 times, most recently from eabdc5e to 75400d3 Compare September 23, 2020 05:51

lxynov changed the title ~~[WIP] avro.schema.literal support and some related patches~~ avro.schema.literal support and some related patches Sep 23, 2020

lxynov marked this pull request as ready for review September 23, 2020 05:52

lxynov requested review from jiegzhan and findepi September 23, 2020 21:28

findepi reviewed Oct 16, 2020

View reviewed changes

ebyhr self-requested a review December 3, 2020 06:27

lxynov force-pushed the avro branch 2 times, most recently from c68ea1c to 96ce3ef Compare December 17, 2020 04:20

ebyhr reviewed Jan 1, 2021

View reviewed changes

lxynov force-pushed the avro branch 2 times, most recently from c175748 to 7c82037 Compare January 7, 2021 04:42

lxynov force-pushed the avro branch from 7c82037 to e87ea83 Compare January 7, 2021 17:44

lxynov added 5 commits January 7, 2021 11:42

Let MockThriftMetastoreClient#getFields return TEST_COLUMN

85ae07e

Call getFields for serdes not using use metastore for schema

6d47778

Use table-level schema for partitions whose Serdes don't use metastor…

f1b0dbe

…e for schema

Allow creating tables with Avro schema literal

bb285a2

Use table-level SerDe parameters for Avro partitions

32e1e4b

lxynov force-pushed the avro branch from e87ea83 to 32e1e4b Compare January 7, 2021 19:43

findepi force-pushed the master branch from 8538e49 to 1f896ea Compare July 30, 2021 22:13

weijiii mentioned this pull request Oct 3, 2022

Allow creating tables with avro.schema.literal #14426

Merged

colebow closed this Nov 11, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avro.schema.literal support and some related patches #5101

avro.schema.literal support and some related patches #5101

lxynov commented Sep 8, 2020 •

edited

Loading

jiegzhan commented Sep 18, 2020

lxynov commented Sep 23, 2020 •

edited

Loading

jiegzhan commented Oct 13, 2020

findepi Oct 16, 2020

lxynov Dec 17, 2020

findepi Oct 16, 2020

lxynov Dec 16, 2020

findepi Oct 16, 2020

lxynov Dec 17, 2020

findepi commented Oct 16, 2020

lxynov commented Dec 17, 2020

ebyhr left a comment

ebyhr Jan 1, 2021

lxynov Jan 7, 2021

lxynov commented Jan 7, 2021

lxynov commented Jan 7, 2021

lxynov commented Jan 19, 2021

colebow commented Oct 19, 2022

avro.schema.literal support and some related patches #5101

avro.schema.literal support and some related patches #5101

Conversation

lxynov commented Sep 8, 2020 • edited Loading

jiegzhan commented Sep 18, 2020

lxynov commented Sep 23, 2020 • edited Loading

jiegzhan commented Oct 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Oct 16, 2020

lxynov commented Dec 17, 2020

ebyhr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lxynov commented Jan 7, 2021

lxynov commented Jan 7, 2021

lxynov commented Jan 19, 2021

colebow commented Oct 19, 2022

lxynov commented Sep 8, 2020 •

edited

Loading

lxynov commented Sep 23, 2020 •

edited

Loading