Skip to content

[Coral-Common] Convert Hive uniontype into a struct-RelDataType that conforms Trino' schema #192

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
wmoustafa merged 12 commits into from
Nov 19, 2021
Merged

[Coral-Common] Convert Hive uniontype into a struct-RelDataType that conforms Trino' schema #192

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ad2bc8f
Convert Hive uniontype into a struct-relDataType that conforms Trino'…
autumnust Nov 9, 2021
33cdf38
Avoid adding additional fields in the exploded struct if union is emp…
autumnust Nov 9, 2021
2d91f48
apply spotless fix
autumnust Nov 9, 2021
4f7bf56
Address reviewer's comments
autumnust Nov 11, 2021
f0f64a9
Address reviewer's comments
autumnust Nov 16, 2021
c77fb85
Run the spotless
autumnust Nov 16, 2021
d4cd58e
Fix unit tests failure
autumnust Nov 16, 2021
cfb3298
Spotless apply through all modules
autumnust Nov 16, 2021
612a362
Fix the tag field in trino'struct to be tinyint
autumnust Nov 16, 2021
40eb25e
Simplified the test validation by using stringfy method
autumnust Nov 17, 2021
c5210cb
Remove uneeded test table
autumnust Nov 17, 2021
2493a8a
Get rid of size check to keep consistent with Trino's handling when f…
autumnust Nov 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion coral-common/src/main/java/com/linkedin/coral/common/TypeConverter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,11 +141,16 @@ public static RelDataType convert(StructTypeInfo structType, final RelDataTypeFa
}

// Mimic the StructTypeInfo conversion to convert a UnionTypeInfo to the corresponding RelDataType
// The schema of output Struct conforms to https://github.com/trinodb/trino/pull/3483
public static RelDataType convert(UnionTypeInfo unionType, RelDataTypeFactory dtFactory) {
List<RelDataType> fTypes = unionType.getAllUnionObjectTypeInfos().stream()
.map(typeInfo -> convert(typeInfo, dtFactory)).collect(Collectors.toList());
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) {

@rzhang10 rzhang10 Nov 9, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@autumnust autumnust Nov 10, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

@wmoustafa wmoustafa Nov 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@autumnust autumnust Nov 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

@rzhang10 rzhang10 Nov 16, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wmoustafa wmoustafa Nov 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@funcheetah funcheetah Nov 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@autumnust autumnust Nov 17, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ljfgem ljfgem Nov 17, 2021

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to the current approach

@wmoustafa wmoustafa Nov 19, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

@ljfgem ljfgem Nov 16, 2021 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 😉

@autumnust autumnust Nov 16, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@funcheetah funcheetah Nov 16, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

fNames.add(0, "tag");
}

RelDataType rowType = dtFactory.createStructType(fTypes, fNames);
return dtFactory.createTypeWithNullability(rowType, true);
Expand Down
23 changes: 22 additions & 1 deletion coral-hive/src/test/java/com/linkedin/coral/hive/hive2rel/HiveTableTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,16 @@
import org.apache.hadoop.hive.metastore.IMetaStoreClient;
import org.apache.hadoop.hive.ql.metadata.HiveException;
import org.apache.thrift.TException;
import org.testng.Assert;
import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;

import com.linkedin.coral.common.HiveSchema;
import com.linkedin.coral.common.HiveTable;

import static org.testng.Assert.*;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertTrue;


public class HiveTableTest {
Expand Down Expand Up @@ -65,6 +68,24 @@ public void testTable() throws Exception {
assertEquals(colC.getType().getComponentType().getSqlTypeName(), SqlTypeName.DOUBLE);
}

@Test
public void testTableWithUnion() throws Exception {
final RelDataTypeFactory typeFactory = new JavaTypeFactoryImpl();

// test handling of union
Comment thread
autumnust marked this conversation as resolved.
Table unionTable = getTable("default", "union_table");
// union_table:(foo uniontype<int, double, array<string>, struct<a:int,b:string>>)
// expected outcome schema: struct<tag:int, field0:int, field1:double, field2:array<string>, field3:struct<a:int,b:string>>
RelDataType rowType = unionTable.getRowType(typeFactory);
assertNotNull(rowType);

// loading equivalent exploded table
Table explodedUnionTable = getTable("default", "exploded_union");
RelDataType rowType1 = explodedUnionTable.getRowType(typeFactory);
assertNotNull(rowType1);
Assert.assertEquals(rowType, rowType1);
}

@Test
public void testGetDaliFunctionParams() throws HiveException, TException {
{
Expand Down
5 changes: 4 additions & 1 deletion coral-hive/src/test/java/com/linkedin/coral/hive/hive2rel/TestUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,15 @@ public static TestHive setupDefaultHive() throws IOException {

driver.run(
"CREATE TABLE IF NOT EXISTS union_table(foo uniontype<int, double, array<string>, struct<a:int,b:string>>)");
driver.run("CREATE TABLE IF NOT EXISTS exploded_union(foo struct<tag:int, field0:int, field1:double, "
+ "field2:array<string>, field3:struct<a:int,b:string>>)");

testHive.databases =
ImmutableList.of(new TestHive.DB("test", ImmutableList.of("tableOne", "tableTwo", "tableOneView")),
new TestHive.DB("default",
ImmutableList.of("bar", "complex", "foo", "foo_view", "null_check_view", "null_check_wrapper",
"schema_evolve", "view_schema_evolve", "view_schema_evolve_wrapper", "union_table")),
"schema_evolve", "view_schema_evolve", "view_schema_evolve_wrapper", "union_table",
"exploded_union")),
new TestHive.DB("fuzzy_union",
ImmutableList.of("tableA", "tableB", "tableC", "union_view", "union_view_with_more_than_two_tables",
"union_view_with_alias", "union_view_single_branch_evolved",
Expand Down
2 changes: 1 addition & 1 deletion coral-spark/src/test/java/com/linkedin/coral/spark/CoralSparkTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ public void testSchemaPromotionView() {
assertEquals(CoralSpark.create(relNode).getSparkSql(), targetSql);
}

@Test
@Test(enabled = false)
public void testUnionExtractUDF() {
RelNode relNode = TestUtils.toRelNode("SELECT extract_union(foo) from union_table");
String targetSql = String.join("\n", "SELECT foo", "FROM default.union_table");
Expand Down