[SPARK-28472][SQL][TEST] Add test for thriftserver protocol versions by wangyum · Pull Request #25228 · apache/spark

wangyum · 2019-07-22T11:23:09Z

What changes were proposed in this pull request?

This pr adds a test(SparkThriftServerProtocolVersionsSuite) to test different versions of the thrift protocol because we use different logic to handle the RowSet:

spark/sql/hive-thriftserver/v1.2.1/src/main/java/org/apache/hive/service/cli/RowSetFactory.java

Lines 28 to 40 in 02c3369

    
           public static RowSet create(TableSchema schema, TProtocolVersion version) { 
        
             if (version.getValue() >= HIVE_CLI_SERVICE_PROTOCOL_V6.getValue()) { 
        
               return new ColumnBasedSet(schema); 
        
             } 
        
             return new RowBasedSet(schema); 
        
           } 
        
           public static RowSet create(TRowSet results, TProtocolVersion version) { 
        
             if (version.getValue() >= HIVE_CLI_SERVICE_PROTOCOL_V6.getValue()) { 
        
               return new ColumnBasedSet(results); 
        
             } 
        
             return new RowBasedSet(results); 
        
           }

When adding this test cases, found three bugs:
SPARK-26969: Using ODBC not able to see the data in table when datatype is decimal
SPARK-28463: Thriftserver throws BigDecimal incompatible with HiveDecimal
SPARK-28474: Lower JDBC client version(Hive 0.12) cannot read binary type

How was this patch tested?

N/A

SparkQA · 2019-07-22T11:32:33Z

Test build #108009 has finished for PR 25228 at commit a8f0ad3.

This patch fails to build.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-22T12:32:59Z

Test build #108012 has finished for PR 25228 at commit d1f4d26.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2019-07-22T12:40:13Z

retest this please

SparkQA · 2019-07-22T13:05:59Z

Test build #108013 has finished for PR 25228 at commit d1f4d26.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-22T15:45:06Z

Test build #108018 has finished for PR 25228 at commit 3fa6e53.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

wangyum · 2019-07-22T22:50:58Z

cc @juliuszsompolski @gatorsmile

...st/scala/org/apache/spark/sql/hive/thriftserver/SparkThriftServerProtocolVersionsSuite.scala

SparkQA · 2019-07-23T02:13:43Z

Test build #108028 has finished for PR 25228 at commit 00c4c69.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-07-23T10:34:08Z

Test build #108048 has finished for PR 25228 at commit 7d1cc80.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

srowen · 2019-07-26T00:44:16Z

...ver/v1.2.1/src/main/scala/org/apache/spark/sql/hive/thriftserver/ThriftserverShimUtils.scala


  private[thriftserver] def toJavaSQLType(s: String): Int = Type.getType(s).toJavaSQLType

+  private[thriftserver] val testedProtocalVersions = Seq(


Protocal -> Protocol
Do we really have to test against so many versions? how many are in use by versions of Hive supported by 3.0?

Two options:

HIVE_CLI_SERVICE_PROTOCOL_V3 -> HIVE_CLI_SERVICE_PROTOCOL_V8 because we support Hive 0.12.

HIVE_CLI_SERVICE_PROTOCOL_V1 -> HIVE_CLI_SERVICE_PROTOCOL_V8. Like Hive, lower versions of the client can always connect to a higher version of ThriftServer. e.g. Hive 0.11 still can connect to Hive 3.1's ThriftServer without any warning:

[root@spark-3267648 hive-0.11.0-bin]# bin/beeline -u jdbc:hive2://localhost:10000/default scan complete in 3ms Connecting to jdbc:hive2://localhost:10000/default SLF4J: Class path contains multiple SLF4J bindings. SLF4J: Found binding in [jar:file:/usr/lib/hadoop-2.7.7/share/hadoop/common/lib/slf4j-log4j12-1.7.10.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: Found binding in [jar:file:/root/hive-0.11.0-bin/lib/slf4j-log4j12-1.6.1.jar!/org/slf4j/impl/StaticLoggerBinder.class] SLF4J: See http://www.slf4j.org/codes.html#multiple_bindings for an explanation. SLF4J: Actual binding is of type [org.slf4j.impl.Log4jLoggerFactory] Connected to: Hive (version 3.1.1) Driver: Hive (version 0.11.0) Transaction isolation: TRANSACTION_REPEATABLE_READ Beeline version 0.11.0 by Apache Hive 0: jdbc:hive2://localhost:10000/default> select 1; +------+ | _c0 | +------+ | 1 | +------+ 1 row selected (0.291 seconds)

I'd prefer option 2.

I guess the downside is we spend extra time testing more protocols? would we ever care to fix support of such old protocols if they didn't work?

How about test HIVE_CLI_SERVICE_PROTOCOL_V3 -> HIVE_CLI_SERVICE_PROTOCOL_V8 because we support Hive 0.12?

I trust your judgment either way, but that seems reasonable to me.

SparkQA · 2019-07-26T17:07:25Z

Test build #108231 has finished for PR 25228 at commit bde0129.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

gatorsmile · 2019-07-26T23:38:59Z

cc @juliuszsompolski @bogdanrdc

juliuszsompolski

LGTM with minor question/nit

juliuszsompolski · 2019-07-27T09:47:50Z

...st/scala/org/apache/spark/sql/hive/thriftserver/SparkThriftServerProtocolVersionsSuite.scala

+    // We do not fully support interval type
+    ignore(s"$version get interval type") {
+      testWithProtocolVersion(version, "SELECT interval '1' year '2' day") { rs =>
+        assert(rs.next())


nit: should we still assert what the returned value is equal to here?

it will throw exception.
Client log:

0: jdbc:hive2://localhost:10000/default> SELECT interval '1' year '2' day; Error: java.lang.IllegalArgumentException: Unrecognized type name: interval (state=,code=0)

Server log:

java.lang.RuntimeException: java.lang.IllegalArgumentException: Unrecognized type name: interval at org.apache.hive.service.cli.session.HiveSessionProxy.invoke(HiveSessionProxy.java:83) at org.apache.hive.service.cli.session.HiveSessionProxy.access$000(HiveSessionProxy.java:36) at org.apache.hive.service.cli.session.HiveSessionProxy$1.run(HiveSessionProxy.java:63) at java.security.AccessController.doPrivileged(AccessController.java:770) at javax.security.auth.Subject.doAs(Subject.java:422) at org.apache.hadoop.security.UserGroupInformation.doAs(UserGroupInformation.java:1746) at org.apache.hive.service.cli.session.HiveSessionProxy.invoke(HiveSessionProxy.java:59) at com.sun.proxy.$Proxy26.getResultSetMetadata(Unknown Source) at org.apache.hive.service.cli.CLIService.getResultSetMetadata(CLIService.java:436) at org.apache.hive.service.cli.thrift.ThriftCLIService.GetResultSetMetadata(ThriftCLIService.java:607) at org.apache.hive.service.cli.thrift.TCLIService$Processor$GetResultSetMetadata.getResult(TCLIService.java:1533) at org.apache.hive.service.cli.thrift.TCLIService$Processor$GetResultSetMetadata.getResult(TCLIService.java:1518) at org.apache.thrift.ProcessFunction.process(ProcessFunction.java:38) at org.apache.thrift.TBaseProcessor.process(TBaseProcessor.java:39) at org.apache.hive.service.auth.TSetIpAddressProcessor.process(TSetIpAddressProcessor.java:53) at org.apache.thrift.server.TThreadPoolServer$WorkerProcess.run(TThreadPoolServer.java:310) at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) at java.lang.Thread.run(Thread.java:819) Caused by: java.lang.IllegalArgumentException: Unrecognized type name: interval at org.apache.hive.service.cli.Type.getType(Type.java:169) at org.apache.hive.service.cli.TypeDescriptor.<init>(TypeDescriptor.java:53) at org.apache.hive.service.cli.ColumnDescriptor.<init>(ColumnDescriptor.java:53) at org.apache.hive.service.cli.TableSchema.<init>(TableSchema.java:52) at org.apache.spark.sql.hive.thriftserver.SparkExecuteStatementOperation$.getTableSchema(SparkExecuteStatementOperation.scala:314) at org.apache.spark.sql.hive.thriftserver.SparkExecuteStatementOperation.resultSchema$lzycompute(SparkExecuteStatementOperation.scala:69) at org.apache.spark.sql.hive.thriftserver.SparkExecuteStatementOperation.resultSchema(SparkExecuteStatementOperation.scala:64) at org.apache.spark.sql.hive.thriftserver.SparkExecuteStatementOperation.getResultSetSchema(SparkExecuteStatementOperation.scala:158) at org.apache.hive.service.cli.operation.OperationManager.getOperationResultSetSchema(OperationManager.java:209) at org.apache.hive.service.cli.session.HiveSessionImpl.getResultSetMetadata(HiveSessionImpl.java:773) at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method) at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62) at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43) at java.lang.reflect.Method.invoke(Method.java:498) at org.apache.hive.service.cli.session.HiveSessionProxy.invoke(HiveSessionProxy.java:78) ... 18 more

Would it fix it if it was converted to string like other unsupported types (Arrays, Maps etc.) in https://github.com/apache/spark/blob/master/sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkExecuteStatementOperation.scala#L107 ?

juliuszsompolski · 2019-07-27T09:57:09Z

Sorry, more comments post LGTM:

Regarding testing V3 to V10 vs V1 to V10:
Simba drivers seem to connect initially with V1, make some initial calls (set -v to get all configs, GetInfo calls to get CLI_SERVER_NAME, CLI_DBMS_VER), and then it disconnects and reconnects with a higher version (V8 for thriftserver v1.2.1).

Maybe we could test starting to V1 then, and add some tests for other requests than executing queries (GetInfo calls, some metadata requests...)?

SparkQA · 2019-07-30T02:24:56Z

Test build #108359 has finished for PR 25228 at commit 1c9afae.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

juliuszsompolski · 2019-07-30T10:48:00Z

LGTM. Thanks!

juliuszsompolski · 2019-07-31T15:44:38Z

cc @gatorsmile

gatorsmile · 2019-08-03T01:49:07Z

retest this please

gatorsmile

LGTM pending Jenkins.

SparkQA · 2019-08-03T02:18:36Z

Test build #108593 has finished for PR 25228 at commit 1c9afae.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-08-06T05:51:11Z

Test build #108696 has finished for PR 25228 at commit b44804a.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2019-08-06T05:58:21Z

Test build #108697 has finished for PR 25228 at commit 1c9afae.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

Add SparkThriftServerProtocolVersionsSuite

a8f0ad3

wangyum mentioned this pull request Jul 22, 2019

[SPARK-26969][SQL] Using ODBC client not able to see the query data when column datatype is decimal #23899

Closed

Fix build error

d1f4d26

wangyum changed the title ~~[SPARK-28472][SQL][TEST] Add test for thriftserver protocol versions~~ [SPARK-28472][SQL][TEST][test-hadoop3.2] Add test for thriftserver protocol versions Jul 22, 2019

Add more test

3fa6e53

dongjoon-hyun added SQL TESTS labels Jul 22, 2019

HyukjinKwon reviewed Jul 23, 2019

View reviewed changes

...st/scala/org/apache/spark/sql/hive/thriftserver/SparkThriftServerProtocolVersionsSuite.scala Outdated Show resolved Hide resolved

Minor fix

00c4c69

Add more test(char and varchar)

7d1cc80

srowen reviewed Jul 26, 2019

View reviewed changes

HIVE_CLI_SERVICE_PROTOCOL_V3 -> HIVE_CLI_SERVICE_PROTOCOL_V8

bde0129

juliuszsompolski approved these changes Jul 27, 2019

View reviewed changes

Add testGetInfoWithProtocolVersion and testGetTablesWithProtocolVersion

1c9afae

juliuszsompolski approved these changes Jul 30, 2019

View reviewed changes

gatorsmile changed the title ~~[SPARK-28472][SQL][TEST][test-hadoop3.2] Add test for thriftserver protocol versions~~ [SPARK-28472][SQL][TEST] Add test for thriftserver protocol versions Aug 3, 2019

gatorsmile reviewed Aug 3, 2019

View reviewed changes

gatorsmile closed this in a59fdc4 Aug 7, 2019

wangyum deleted the SPARK-28472 branch August 7, 2019 15:57

	public static RowSet create(TableSchema schema, TProtocolVersion version) {
	if (version.getValue() >= HIVE_CLI_SERVICE_PROTOCOL_V6.getValue()) {
	return new ColumnBasedSet(schema);
	}
	return new RowBasedSet(schema);
	}

	public static RowSet create(TRowSet results, TProtocolVersion version) {
	if (version.getValue() >= HIVE_CLI_SERVICE_PROTOCOL_V6.getValue()) {
	return new ColumnBasedSet(results);
	}
	return new RowBasedSet(results);
	}


		private[thriftserver] def toJavaSQLType(s: String): Int = Type.getType(s).toJavaSQLType

		private[thriftserver] val testedProtocalVersions = Seq(

Conversation

wangyum commented Jul 22, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

SparkQA commented Jul 22, 2019

Uh oh!

SparkQA commented Jul 22, 2019

Uh oh!

wangyum commented Jul 22, 2019

Uh oh!

SparkQA commented Jul 22, 2019

Uh oh!

SparkQA commented Jul 22, 2019

Uh oh!

wangyum commented Jul 22, 2019

Uh oh!

Uh oh!

SparkQA commented Jul 23, 2019

Uh oh!

SparkQA commented Jul 23, 2019

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!

SparkQA commented Jul 26, 2019

Uh oh!

gatorsmile commented Jul 26, 2019

Uh oh!

juliuszsompolski left a comment

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!

juliuszsompolski commented Jul 27, 2019

Uh oh!

SparkQA commented Jul 30, 2019

Uh oh!

juliuszsompolski commented Jul 30, 2019

Uh oh!

juliuszsompolski commented Jul 31, 2019

Uh oh!

gatorsmile commented Aug 3, 2019

Uh oh!

gatorsmile left a comment

Choose a reason for hiding this comment

Uh oh!

SparkQA commented Aug 3, 2019

Uh oh!

SparkQA commented Aug 6, 2019

Uh oh!

SparkQA commented Aug 6, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

wangyum commented Jul 22, 2019 •

edited

Loading