[BugFix]Only when unsupported column is referred, exception will be thrown. #8881

adzfolc · 2022-07-18T15:49:05Z

What type of PR is this：

Which issues of this PR fixes ：

Problem Summary(Required) ：

Table binarytest has two columns, col_int and col_binary, and only col_int is unsupported.
The goal is that, only when unsupported column is referred, exception will be thrown.

mysql> select * from binarytest;
ERROR 1064 (HY000): Column col_binary convert failed!
mysql> select col_binary from binarytest;
ERROR 1064 (HY000): Column col_binary convert failed!
mysql> select col_int from binarytest;
+---------+
| col_int |
+---------+
|     100 |
|     100 |
+---------+
2 rows in set (0.95 sec)

mysql> select col_int from binarytest where col_binary != null;
ERROR 1064 (HY000): Column col_binary convert failed!

…xception would be thrown 1. add `CONVERT_FAILED` type to primitive type 2. add hive unsupported column convert UT 3. add more code comments

adzfolc · 2022-07-19T02:41:05Z

PTAL @DorianZheng @mofeiatwork @HangyuanLiu

DorianZheng · 2022-07-19T07:17:46Z

fe/fe-core/src/main/java/com/starrocks/external/HiveMetaStoreTableUtils.java

@@ -156,6 +156,9 @@ public static boolean validateColumnType(String hiveType, Type type) {
                        primitiveType == PrimitiveType.VARCHAR;
            case "BOOLEAN":
                return primitiveType == PrimitiveType.BOOLEAN;
+            case "BINARY":


validateColumnType is used to validate external table schema when creation, so we can just return fail when types are unsupported

validateColumnType is used to validate external table schema when creation, so we can just return fail when types are unsupported

All queries related to external hive tables need to validate hive column, so if we return false when table column is unsupported, even if the query has no relation with unsupported type, will fail here.

DorianZheng · 2022-07-19T07:19:15Z

fe/fe-core/src/main/java/com/starrocks/external/HiveMetaStoreTableUtils.java

@@ -210,6 +213,9 @@ public static Type convertColumnType(String hiveType) throws DdlException {
            case "BOOLEAN":
                primitiveType = PrimitiveType.BOOLEAN;
                break;
+            case "BINARY":


Not only Binary. All unsupported types should be captured.

Not only Binary. All unsupported types should be captured.

case "BINARY": case "MAP": case "STRUCT": case "UNIONTYPE": primitiveType = PrimitiveType.UNKNOWN_TYPE; break;

Now complex types and binary type will all be converted to UNKNOWN_TYPE.

why not capture it in default branch?

DorianZheng · 2022-07-19T07:21:54Z

@adzfolc Plz add more ut to cover more case

HangyuanLiu · 2022-07-19T09:43:51Z

why not use INVALID_TYPE?

adzfolc · 2022-07-19T12:33:30Z

why not use INVALID_TYPE?

In my opinion, Invalid and convert failed have different semantics. Invalid is an invalid column, and invalid type is not allowed in any table; for convert failed column, it is allowed to have this type in a table, but it is not allowed to exist in q query, as long as there is this column in sql, then an error will be reported.

2. rename column type 'CONVERT_FAILED' to 'UNKNOWN_TYPE' 3. revert Be and thrift modification

adzfolc · 2022-07-25T12:00:50Z

run starrocks_fe_unittest

adzfolc · 2022-07-25T12:14:36Z

PR Regression Link http://39.101.206.100:8090/view/regression/job/starrocks_pr_regression/626/

fe/fe-core/src/main/java/com/starrocks/qe/StmtExecutor.java

fe/fe-core/src/main/java/com/starrocks/sql/plan/PlanFragmentBuilder.java

imay · 2022-07-26T01:31:08Z

fe/fe-core/src/main/java/com/starrocks/catalog/Type.java

@@ -149,6 +150,7 @@ public abstract class Type implements Cloneable {
                    .add(DECIMAL64)
                    .add(DECIMAL128)
                    .add(JSON)
+                    .add(UNKNOWN_TYPE)


why unknown type is added into supported types?

why unknown type is added into supported types?

Because when Fe analzy table, it need to resolve table. All hive types would be converted to sr types, including UNKNOWN_TYPE. In code, ScalarType.createType() will find UNKNOWN_TYPE in SUPPORTED_TYPES, if it is removed, exception will be thrown like below.

2022-07-26 10:47:24,907 ERROR (starrocks-mysql-nio-pool-1|150) [HiveMetaCache.getTable():315] Failed to get table [databaseName: old_test, tableName: binarytest] com.google.common.util.concurrent.UncheckedExecutionException: java.lang.NullPointerException: unknown type UNKNOWN_TYPE at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2051) ~[spark-dpp-1.0.0.jar:?] at com.google.common.cache.LocalCache.get(LocalCache.java:3962) ~[spark-dpp-1.0.0.jar:?] at com.google.common.cache.LocalCache.getOrLoad(LocalCache.java:3985) ~[spark-dpp-1.0.0.jar:?] at com.google.common.cache.LocalCache$LocalLoadingCache.get(LocalCache.java:4946) ~[spark-dpp-1.0.0.jar:?] at com.starrocks.external.hive.HiveMetaCache.getTable(HiveMetaCache.java:313) ~[starrocks-fe.jar:?] at com.starrocks.connector.hive.HiveMetadata.getTable(HiveMetadata.java:61) ~[starrocks-fe.jar:?] at com.starrocks.server.MetadataMgr.lambda$getTable$1(MetadataMgr.java:114) ~[starrocks-fe.jar:?] at java.util.Optional.map(Optional.java:215) ~[?:1.8.0_321] at com.starrocks.server.MetadataMgr.getTable(MetadataMgr.java:114) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer.resolveTable(QueryAnalyzer.java:669) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer.access$100(QueryAnalyzer.java:66) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.resolveTableRef(QueryAnalyzer.java:235) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.visitSelect(QueryAnalyzer.java:161) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.visitSelect(QueryAnalyzer.java:83) ~[starrocks-fe.jar:?] at com.starrocks.sql.ast.SelectRelation.accept(SelectRelation.java:206) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.process(QueryAnalyzer.java:88) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.visitQueryRelation(QueryAnalyzer.java:106) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.visitQueryStatement(QueryAnalyzer.java:100) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.visitQueryStatement(QueryAnalyzer.java:83) ~[starrocks-fe.jar:?] at com.starrocks.sql.ast.QueryStatement.accept(QueryStatement.java:36) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer$Visitor.process(QueryAnalyzer.java:88) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.QueryAnalyzer.analyze(QueryAnalyzer.java:76) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.Analyzer$AnalyzerVisitor.visitQueryStatement(Analyzer.java:204) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.Analyzer$AnalyzerVisitor.visitQueryStatement(Analyzer.java:68) ~[starrocks-fe.jar:?] at com.starrocks.sql.ast.QueryStatement.accept(QueryStatement.java:36) ~[starrocks-fe.jar:?] at com.starrocks.sql.ast.AstVisitor.visit(AstVisitor.java:104) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.Analyzer$AnalyzerVisitor.analyze(Analyzer.java:70) ~[starrocks-fe.jar:?] at com.starrocks.sql.analyzer.Analyzer.analyze(Analyzer.java:65) ~[starrocks-fe.jar:?] at com.starrocks.sql.StatementPlanner.plan(StatementPlanner.java:43) ~[starrocks-fe.jar:?] at com.starrocks.qe.StmtExecutor.execute(StmtExecutor.java:346) ~[starrocks-fe.jar:?] at com.starrocks.qe.ConnectProcessor.handleQuery(ConnectProcessor.java:273) ~[starrocks-fe.jar:?] at com.starrocks.qe.ConnectProcessor.dispatch(ConnectProcessor.java:391) ~[starrocks-fe.jar:?] at com.starrocks.qe.ConnectProcessor.processOnce(ConnectProcessor.java:624) ~[starrocks-fe.jar:?] at com.starrocks.mysql.nio.ReadListener.lambda$handleEvent$0(ReadListener.java:55) ~[starrocks-fe.jar:?] at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_321] at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_321] at java.lang.Thread.run(Thread.java:750) [?:1.8.0_321] Caused by: java.lang.NullPointerException: unknown type UNKNOWN_TYPE at com.google.common.base.Preconditions.checkNotNull(Preconditions.java:899) ~[spark-dpp-1.0.0.jar:?] at com.starrocks.catalog.ScalarType.createType(ScalarType.java:106) ~[starrocks-fe.jar:?] at com.starrocks.external.HiveMetaStoreTableUtils.convertColumnType(HiveMetaStoreTableUtils.java:229) ~[starrocks-fe.jar:?] at com.starrocks.external.HiveMetaStoreTableUtils.convertToSRTable(HiveMetaStoreTableUtils.java:247) ~[starrocks-fe.jar:?] at com.starrocks.external.hive.HiveMetaCache.loadTable(HiveMetaCache.java:323) ~[starrocks-fe.jar:?] at com.starrocks.external.hive.HiveMetaCache.access$700(HiveMetaCache.java:39) ~[starrocks-fe.jar:?] at com.starrocks.external.hive.HiveMetaCache$8.load(HiveMetaCache.java:144) ~[starrocks-fe.jar:?] at com.starrocks.external.hive.HiveMetaCache$8.load(HiveMetaCache.java:141) ~[starrocks-fe.jar:?] at com.google.common.cache.CacheLoader$1.load(CacheLoader.java:192) ~[spark-dpp-1.0.0.jar:?] at com.google.common.cache.LocalCache$LoadingValueReference.loadFuture(LocalCache.java:3529) ~[spark-dpp-1.0.0.jar:?] at com.google.common.cache.LocalCache$Segment.loadSync(LocalCache.java:2278) ~[spark-dpp-1.0.0.jar:?] at com.google.common.cache.LocalCache$Segment.lockedGetOrLoad(LocalCache.java:2155) ~[spark-dpp-1.0.0.jar:?] at com.google.common.cache.LocalCache$Segment.get(LocalCache.java:2045) ~[spark-dpp-1.0.0.jar:?] ... 36 more

I think you should change the convertColumnType method, not here. I think it's unreasonable we make the UNKNOWN_TYPE as the SUPPORTED_TYPES, but we actually don't support it.

I think you should change the convertColumnType method, not here. I think it's unreasonable we make the UNKNOWN_TYPE as the SUPPORTED_TYPES, but we actually don't support it.

In convertColumnType, I transfer hive column type to sr column type, so even if unsupported types like hive Binary should be given a sr column type.So, UNKNOWN_TYPE matches this case.So, UNKNOWN_TYPE should be one of PrimitiveType.It's truly counter-intuitive, but we still need it.

adzfolc · 2022-07-26T09:33:30Z

run starrocks_be_unittest

adzfolc · 2022-07-26T09:36:07Z

PR Regression Link http://39.101.206.100:8090/view/regression/job/starrocks_pr_regression/637/

…lumn type

adzfolc · 2022-07-26T15:59:48Z

run starrocks_be_build

kangkaisen · 2022-07-27T05:32:51Z

fe/fe-core/src/main/java/com/starrocks/catalog/Type.java

@@ -367,6 +378,7 @@ public abstract class Type implements Cloneable {
        compatibilityMatrix[DATETIME.ordinal()][DECIMAL32.ordinal()] = PrimitiveType.DECIMAL32;
        compatibilityMatrix[DATETIME.ordinal()][DECIMAL64.ordinal()] = PrimitiveType.DECIMAL64;
        compatibilityMatrix[DATETIME.ordinal()][DECIMAL128.ordinal()] = PrimitiveType.DECIMAL128;
+        compatibilityMatrix[DATETIME.ordinal()][UNKNOWN_TYPE.ordinal()] = PrimitiveType.INVALID_TYPE;


why do we need to add the UNKNOWN_TYPE to compatibilityMatrix?

why do we need to add the UNKNOWN_TYPE to compatibilityMatrix?

Because UNKNOWN_TYPE is added to PrimitiveType, and types will be compared with UNKNOWN_TYPE.

fe/fe-core/src/main/java/com/starrocks/sql/analyzer/SelectAnalyzer.java

fe/fe-core/src/main/java/com/starrocks/sql/analyzer/Scope.java

…TYPE as default

fe/fe-core/src/main/java/com/starrocks/catalog/Type.java

wanpengfei-git · 2022-07-28T03:25:50Z

[FE PR Coverage Check]

😍 pass : 38 / 41 (92.68%)

file detail

	path	covered_line	new_line	coverage	not_covered_line_detail
🔵	com/starrocks/external/HiveMetaStoreTableUtils.java	1	2	50.00%	[209]
🔵	com/starrocks/sql/analyzer/Scope.java	2	3	66.67%	[58]
🔵	com/starrocks/sql/analyzer/SelectAnalyzer.java	4	5	80.00%	[199]
🔵	com/starrocks/catalog/PrimitiveType.java	2	2	100.00%	[]
🔵	com/starrocks/catalog/HiveTable.java	2	2	100.00%	[]
🔵	com/starrocks/catalog/ScalarType.java	2	2	100.00%	[]
🔵	com/starrocks/catalog/Type.java	25	25	100.00%	[]

sonarcloud · 2022-07-28T03:26:35Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
3 Code Smells

0.0% Coverage
0.0% Duplication

adzfolc · 2022-07-28T03:34:02Z

http://39.101.206.100:8090/view/regression/job/starrocks_pr_regression/653/

wanpengfei-git · 2022-07-28T06:10:10Z

run starrocks_admit_test

Related to #8881 When there are unsupported types in an Iceberg table, exception will not be thrown if users don't select columns with such types. Like the followings: ```sql hive> select * from t.m; OK 1 {"a":"b"} MySQL [(none)]> select * from ice.t.m; ERROR 1064 (HY000): External Table Column [m] convert failed, and column type is known! MySQL [(none)]> select i from ice.t.m; +------+ | i | +------+ | 1 | +------+ 1 row in set (0.08 sec) ```

adzfolc · 2022-08-17T03:45:47Z

@Mergifyio backport branch-2.3

mergify · 2022-08-17T03:46:19Z

backport branch-2.3

✅ Backports have been created

#10095 [BugFix]Only when unsupported column is referred, exception will be thrown. (backport #8881) has been created for branch branch-2.3

…hrown. (#8881) Table binarytest has two columns, col_int and col_binary, and only col_int is unsupported. The goal is that, only when unsupported column is referred, exception will be thrown. (cherry picked from commit b3c4d10)

* [BugFix]Only when unsupported column is referred, exception will be thrown. (#8881) Table binarytest has two columns, col_int and col_binary, and only col_int is unsupported. The goal is that, only when unsupported column is referred, exception will be thrown. (cherry picked from commit b3c4d10)

0. feature: if one column is unsupported, only when it is referred, e…

c1691df

…xception would be thrown 1. add `CONVERT_FAILED` type to primitive type 2. add hive unsupported column convert UT 3. add more code comments

DorianZheng reviewed Jul 19, 2022

View reviewed changes

1. add detailed comment

e98e182

2. rename column type 'CONVERT_FAILED' to 'UNKNOWN_TYPE' 3. revert Be and thrift modification

imay reviewed Jul 26, 2022

View reviewed changes

adzfolc added 2 commits July 26, 2022 16:33

1. check all unknown types when analyzing

0efd660

1. remove unrelated changes

639ba8b

adzfolc added 2 commits July 26, 2022 21:29

1. Only internal catalog like hive external table need to validate co…

0b65915

…lumn type

1. Convert Hive types [BINARY, MAP, STRUCT, UNIONTYPE] to UNKNOWN_TYPES

e8e9b21

Merge branch 'StarRocks:main' into feat-filter-unsupported-type

ac809e3

kangkaisen reviewed Jul 27, 2022

View reviewed changes

fe/fe-core/src/main/java/com/starrocks/sql/analyzer/SelectAnalyzer.java Outdated Show resolved Hide resolved

kangkaisen reviewed Jul 27, 2022

View reviewed changes

fe/fe-core/src/main/java/com/starrocks/sql/analyzer/Scope.java Outdated Show resolved Hide resolved

adzfolc added 5 commits July 27, 2022 17:33

1. Convert [BINARY, MAP, STRUCT, UNIONTYPE] to PrimitiveType.UNKNOWN_…

095b4ca

…TYPE as default

1. Add detailed exception message

15a39c8

1. Revert code

75d082b

1. fix code smells

f28183c

1. fix code smells

b61fa7b

kangkaisen reviewed Jul 27, 2022

View reviewed changes

fe/fe-core/src/main/java/com/starrocks/catalog/Type.java Outdated Show resolved Hide resolved

adzfolc added 2 commits July 28, 2022 11:02

1. change skipCompareTypes to static

b8242af

1. fix code smell

5f4edd2

kangkaisen approved these changes Jul 28, 2022

View reviewed changes

imay approved these changes Jul 28, 2022

View reviewed changes

wanpengfei-git added the Approved Ready to merge label Jul 28, 2022

imay merged commit b3c4d10 into StarRocks:main Jul 28, 2022

adzfolc deleted the feat-filter-unsupported-type branch July 29, 2022 07:00

mxdzs0612 mentioned this pull request Aug 1, 2022

[Feature] Iceberg table supports unknown types #9431

Merged

5 tasks

mergify bot mentioned this pull request Aug 17, 2022

[BugFix]Only when unsupported column is referred, exception will be thrown. (backport #8881) #10095

Closed

adzfolc mentioned this pull request Aug 18, 2022

[Feature]Branch 2.3 cp unknown type #10139

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BugFix]Only when unsupported column is referred, exception will be thrown. #8881

[BugFix]Only when unsupported column is referred, exception will be thrown. #8881

adzfolc commented Jul 18, 2022 •

edited

Loading

adzfolc commented Jul 19, 2022

DorianZheng Jul 19, 2022

adzfolc Jul 19, 2022

DorianZheng Jul 19, 2022

adzfolc Jul 26, 2022

DorianZheng Jul 27, 2022

adzfolc Jul 27, 2022

DorianZheng commented Jul 19, 2022

HangyuanLiu commented Jul 19, 2022

adzfolc commented Jul 19, 2022

adzfolc commented Jul 25, 2022

adzfolc commented Jul 25, 2022

imay Jul 26, 2022

adzfolc Jul 26, 2022

kangkaisen Jul 27, 2022

adzfolc Jul 27, 2022

adzfolc commented Jul 26, 2022

adzfolc commented Jul 26, 2022 •

edited

Loading

adzfolc commented Jul 26, 2022

kangkaisen Jul 27, 2022

adzfolc Jul 27, 2022

wanpengfei-git commented Jul 28, 2022

sonarcloud bot commented Jul 28, 2022

adzfolc commented Jul 28, 2022

wanpengfei-git commented Jul 28, 2022

adzfolc commented Aug 17, 2022

mergify bot commented Aug 17, 2022

[BugFix]Only when unsupported column is referred, exception will be thrown. #8881

[BugFix]Only when unsupported column is referred, exception will be thrown. #8881

Conversation

adzfolc commented Jul 18, 2022 • edited Loading

What type of PR is this：

Which issues of this PR fixes ：

Problem Summary(Required) ：

adzfolc commented Jul 19, 2022

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

DorianZheng commented Jul 19, 2022

HangyuanLiu commented Jul 19, 2022

adzfolc commented Jul 19, 2022

adzfolc commented Jul 25, 2022

adzfolc commented Jul 25, 2022

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

adzfolc commented Jul 26, 2022

adzfolc commented Jul 26, 2022 • edited Loading

adzfolc commented Jul 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wanpengfei-git commented Jul 28, 2022

[FE PR Coverage Check]

file detail

sonarcloud bot commented Jul 28, 2022

adzfolc commented Jul 28, 2022

wanpengfei-git commented Jul 28, 2022

adzfolc commented Aug 17, 2022

mergify bot commented Aug 17, 2022

✅ Backports have been created

adzfolc commented Jul 18, 2022 •

edited

Loading

adzfolc commented Jul 26, 2022 •

edited

Loading