[Coral-Common] Convert Hive uniontype into a struct-RelDataType that conforms Trino' schema by autumnust · Pull Request #192 · linkedin/coral

autumnust · 2021-11-09T01:14:50Z

Background:

Previously to make union type translatable between engines, we represent the union-type in Coral-IR as a struct where each member field of union becomes a subfield within a struct with each carrying the name tag_N. For example we would see uniontype<int,string> becomes something like struct<1,null> or struct<null, "h">. This format is based on Hive's extract_union UDF.
Trino supports natively reading union field by exploding the union into a struct, with a different schema: Support reading uniontype as struct from Avro/ORC Hive tables trinodb/trino#3483
This PR conforms the Coral IR's union-after-explosion representation to Trino's format, so that there won't be type differences between engine's deserialized format versus IR of a view's field.

Also added unit tests for the different union conversion logic.

Updates from long discussion in this PR:

For some of the corner cases like empty union, we decided to keep consistent with Trino's PR where it will convert the empty union into a struct with only one field named as tag.
The tag field is in "TINYINT" type. Note that this type does not exist in Iceberg so we will need further testing to verify the whole flow works.

autumnust · 2021-11-09T01:15:17Z

@rzhang10 @funcheetah Can you take a look when you get a chance? thanks !

ljfgem

Thanks @autumnust !
Could you also add the unit tests for Coral-Trino in HiveToTrinoConverterTest?

coral-trino/src/test/java/com/linkedin/coral/trino/rel2trino/TestUtils.java

coral-spark/src/main/java/com/linkedin/coral/spark/IRRelToSparkRelTransformer.java

rzhang10 · 2021-11-09T23:37:24Z

coral-common/src/main/java/com/linkedin/coral/common/TypeConverter.java

-    List<String> fNames = IntStream.range(0, unionType.getAllUnionObjectTypeInfos().size()).mapToObj(i -> "tag_" + i)
+    List<String> fNames = IntStream.range(0, unionType.getAllUnionObjectTypeInfos().size()).mapToObj(i -> "field" + i)
        .collect(Collectors.toList());
+    if (fNames.size() > 0) {


I think we don't need this if statement since an empty union is a wrong schema, to begin with, maybe we could add a Preconditions.checkState to check it.

currently how do we prevent empty union to appear?
The reason for this branch is to preserve the semantic of the same code block earlier. If we add a precondition check, encountering empty union will result in unchecked exception but it will simply return an empty RelDataType before the change. is that what we want ?

Seems that here https://github.com/trinodb/trino/pull/3483/files# the size is not checked. Does the Avro or ORC standard say anything about this?

ORC spec doesn't mention it while I do see uniontype<> happening in production. Regardless, this check is there to keep parity with the original (or we could end up with an additional tag field).

So if using the current code, uniontype<> ends up in an empty struct? I recall there are some issues with empty struct somewhere, either in Iceberg or in Spark. So I'm not sure if this whole corner case will work or not.
My point is if we explicitly announce this special case's behavior is undefined, then users have the responsibility to create the correct schema, it's a tradeoff between us and the users, I'm willing to accept either way.

Our references should be: 1- what the standard says (again for Avro or ORC) 2- what the Trino transformation does. Hopefully all of them align. If not, we can discuss how to move forward.

Another special case is union with only a single type uniontype<[type]>. We should consider whether we want to support it and make sure its semantics is consistent across Trino and Iceberg implementation

There aren't any spec that I could find regarding this point in either Avro or ORC, and looks like Trino doesn't check the size either.
Taking a step back, I also don't think Coral is in right position to gate such usage. So I am leaning towards let such case pass through (which means a check like fNames.size > 0 is necessary since we see struct<> as a legit type).

I am also OK to add a precondition check to fail the translation. But again, leaning towards the former option.

+1 to the current approach

Discussed offline. The objective is to match the Trino schema. We should remove the check from here since it is not used there.

wmoustafa · 2021-11-13T18:24:42Z

coral-common/src/main/java/com/linkedin/coral/common/TypeConverter.java

-    List<String> fNames = IntStream.range(0, unionType.getAllUnionObjectTypeInfos().size()).mapToObj(i -> "tag_" + i)
+    List<String> fNames = IntStream.range(0, unionType.getAllUnionObjectTypeInfos().size()).mapToObj(i -> "field" + i)
        .collect(Collectors.toList());
+    if (fNames.size() > 0) {


Seems that here https://github.com/trinodb/trino/pull/3483/files# the size is not checked. Does the Avro or ORC standard say anything about this?

coral-hive/src/test/java/com/linkedin/coral/hive/hive2rel/HiveTableTest.java

…s schema

…ty itself

ljfgem

LTGM except the type issue, thanks @autumnust !

ljfgem · 2021-11-16T01:47:16Z

coral-common/src/main/java/com/linkedin/coral/common/TypeConverter.java

+    List<String> fNames = IntStream.range(0, unionType.getAllUnionObjectTypeInfos().size()).mapToObj(i -> "field" + i)
        .collect(Collectors.toList());
+    if (fNames.size() > 0) {
+      fTypes.add(0, dtFactory.createSqlType(SqlTypeName.INTEGER));


According to the corresponding PR:
https://github.com/trinodb/trino/pull/3483/files#diff-6a37b030f26bfeb6eca302ca157050cad2ead0476c1785fad57fc5f14fab88cfR264
the type of tag is TINYINT, rather than INT, so it should be

fTypes.add(0, dtFactory.createSqlType(SqlTypeName.TINYINT));

querying the view will fail if the types mismatch.
I think the test also needs to be modified.

Enhanced coral-trino test suite helped to catch this hidden issue 😉

Wow that's a great catch, thanks ! I will address this.

Nice work for enhanced coral-trino test suite! This is awesome! We can catch more issues before hitting production.

ljfgem · 2021-11-16T18:32:08Z

FYI, enhanced i-test succeeded after fixing the type mismatch issue, we can merge it once all the comments are addressed.

autumnust · 2021-11-17T05:45:58Z

FYI, enhanced i-test succeeded after fixing the type mismatch issue, we can merge it once all the comments are addressed.

I addressed the RowType comparison comment. The remaining discussion is mostly on how does Coral deal with corner cases in struct representation. Let's get some agreement on that, if that's needed before merging.

…acing with empty union

ljfgem reviewed Nov 9, 2021

View reviewed changes

coral-trino/src/test/java/com/linkedin/coral/trino/rel2trino/TestUtils.java Outdated Show resolved Hide resolved

coral-spark/src/main/java/com/linkedin/coral/spark/IRRelToSparkRelTransformer.java Outdated Show resolved Hide resolved

rzhang10 reviewed Nov 9, 2021

View reviewed changes

autumnust mentioned this pull request Nov 10, 2021

Coral-Spark: Build awareness in Coral for coalsce_struct UDF #193

Merged

autumnust force-pushed the union_trino_coalsce branch from 1631e52 to e7fac25 Compare November 11, 2021 00:22

wmoustafa reviewed Nov 13, 2021

View reviewed changes

autumnust added 5 commits November 15, 2021 12:14

Convert Hive uniontype into a struct-relDataType that conforms Trino'…

ad2bc8f

…s schema

Avoid adding additional fields in the exploded struct if union is emp…

33cdf38

…ty itself

apply spotless fix

2d91f48

Address reviewer's comments

4f7bf56

Address reviewer's comments

f0f64a9

autumnust force-pushed the union_trino_coalsce branch from e7fac25 to f0f64a9 Compare November 16, 2021 00:29

autumnust added 3 commits November 15, 2021 16:35

Run the spotless

c77fb85

Fix unit tests failure

d4cd58e

Spotless apply through all modules

cfb3298

ljfgem reviewed Nov 16, 2021

View reviewed changes

Fix the tag field in trino'struct to be tinyint

612a362

Simplified the test validation by using stringfy method

40eb25e

autumnust added 2 commits November 16, 2021 21:55

Remove uneeded test table

c5210cb

Get rid of size check to keep consistent with Trino's handling when f…

2493a8a

…acing with empty union

wmoustafa approved these changes Nov 19, 2021

View reviewed changes

wmoustafa merged commit 4324471 into linkedin:master Nov 19, 2021

Conversation

autumnust commented Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

autumnust commented Nov 9, 2021

Uh oh!

ljfgem left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ljfgem left a comment

Choose a reason for hiding this comment

Uh oh!

ljfgem Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljfgem commented Nov 16, 2021

Uh oh!

autumnust commented Nov 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

autumnust commented Nov 9, 2021 •

edited

Loading

ljfgem Nov 16, 2021 •

edited

Loading