[SPARK-24829][STS]In Spark Thrift Server, CAST AS FLOAT inconsistent with spark-shell or spark-sql #21789

zuotingbing · 2018-07-17T09:27:39Z

What changes were proposed in this pull request?

SELECT CAST('4.56' AS FLOAT)

the result is 4.559999942779541

it should be 4.56 as same as in spark-shell or spark-sql.

How was this patch tested?

add unit tests

xuanyuanking · 2018-07-17T14:11:00Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

@@ -766,6 +774,14 @@ class HiveThriftHttpServerSuite extends HiveThriftJdbcTest {
      assert(resultSet.getString(2) === HiveUtils.builtinHiveVersion)
    }
  }
+
+  test("Checks cast as float") {


Duplicated code?

for two different modes: binary & http

then probably better to add it into HiveThriftJdbcTest?

HiveThriftJdbcTest is an abstract class，there are two classess HiveThriftBinaryServerSuite & HiveThriftBinaryServerSuite extends from HiveThriftJdbcTest.

yes, so if you add the test to HiveThriftJdbcTest it will be executed in both in HiveThriftBinaryServerSuite and HiveThriftBinaryServerSuite....

sorry i do not agree with you. HiveThriftBinaryServerSuite and HiveThriftHttpServerSuite ,which have exist before, are different cases for different ServerMode, we should test them all. if we add test in HiveThriftJdbcTest , we need to set different ServerMode in HiveThriftJdbcTest for the two cases, which break the design of abstract class of HiveThriftJdbcTest .

I don't think so. If you add:

test("Checks cast as float") { withJdbcStatement() { statement => val resultSet = statement.executeQuery("SELECT CAST('4.56' AS FLOAT)") resultSet.next() assert(resultSet.getString(1) === "4.56") } }

to the abstract class you need to change nothing else and the test would be executed both as a part of HiveThriftHttpServerSuite with one mode and as a part of HiveThriftBinaryServerSuite with the other mode.

oh i see, i will fix it now. Thanks.

dongjoon-hyun

@zuotingbing. Could you narrow down the scope of title more specifically because this only happens in STS? All other shells like spark-shell and spark-sql works as expected. We had better call this as Spark's inconsistency.

zuotingbing · 2018-07-18T03:03:20Z

yes, only in STS, i will update the title

mgaido91 · 2018-07-18T10:02:46Z

...ftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/HiveThriftServer2Suites.scala

@@ -766,6 +774,14 @@ class HiveThriftHttpServerSuite extends HiveThriftJdbcTest {
      assert(resultSet.getString(2) === HiveUtils.builtinHiveVersion)
    }
  }
+
+  test("Checks cast as float") {


then probably better to add it into HiveThriftJdbcTest?

mgaido91 · 2018-07-18T10:03:47Z

sql/hive-thriftserver/src/main/java/org/apache/hive/service/cli/Column.java

@@ -349,7 +349,7 @@ public void addValue(Type type, Object field) {
        break;
      case FLOAT_TYPE:
        nulls.set(size, field == null);
-        doubleVars()[size] = field == null ? 0 : ((Float)field).doubleValue();
+        doubleVars()[size] = field == null ? 0 : new Double(field.toString());


if the problem is the precision, isn't enough to cast it to Double instead of creating a double out of a string?

sorry i am not sure what is your meaning. use “Double.parseDouble(String.valueOf(field))” ？

I mean ((Double)field)

oh you are wrong. we cannot cast Float to Double like this.

I see now the issue, so we cannot convert directly from float to double, we have to pass through another object which may be a string as in this your solution. Please disregard my comment, thanks.

mgaido91 · 2018-07-20T07:49:46Z

@cloud-fan @gatorsmile this change seems reasonable to me. Do you think we can trigger a build? Thanks.

mgaido91 · 2018-07-20T09:04:37Z

@zuotingbing I think what @dongjoon-hyun was suggesting you was to put [STS] instead of [SQL] in the title of the PR. May you please update accordingly? Thanks.

zuotingbing · 2018-07-20T09:26:27Z

@mgaido91 ok, update it. Thanks.

HyukjinKwon · 2018-07-21T08:46:55Z

ok to test

SparkQA · 2018-07-21T09:21:58Z

Test build #93383 has finished for PR 21789 at commit ffcff14.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

zuotingbing · 2018-07-23T07:59:46Z

@HyukjinKwon i am not sure what is the reason about the tests failed. i checked the unit-tests.log and it seems the test "SPARK-24829 Checks cast as float" , which i added, have finished with no error. Could you give a help to check it or tell me how to fix the failure please? Thanks。

HyukjinKwon · 2018-07-23T08:14:39Z

retest this please

SparkQA · 2018-07-23T08:53:04Z

Test build #93438 has finished for PR 21789 at commit ffcff14.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

zuotingbing · 2018-07-23T09:21:02Z

retest this please @HyukjinKwon

mgaido91 · 2018-07-23T09:49:43Z

retest this please

HyukjinKwon · 2018-07-23T09:54:19Z

retest this please

HyukjinKwon · 2018-07-23T09:54:30Z

ok to test

SparkQA · 2018-07-23T12:43:09Z

Test build #93442 has finished for PR 21789 at commit 05cbdc8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2018-07-23T12:44:45Z

Test build #93440 has finished for PR 21789 at commit 05cbdc8.

This patch fails Spark unit tests.
This patch merges cleanly.
This patch adds no public classes.

zuotingbing · 2018-07-24T01:00:44Z

@gatorsmile org.apache.spark.sql.hive.thriftserver.UISeleniumSuite.(It is not a test it is a sbt.testing.SuiteSelector) , Could you give a help to check it please? i find you are the latest person who update the UISeleniumSuite class , Thanks.

HyukjinKwon · 2018-07-24T01:43:13Z

I think you better take a look for it because that looks related with the current change.

…iftJdbcTest" which makes the UISeleniumSuite test failed

SparkQA · 2018-07-24T06:38:54Z

Test build #93482 has finished for PR 21789 at commit 88dff75.

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

mgaido91 · 2018-07-25T10:59:20Z

LGTM, I checked and the same hack is done also in Hive.

zuotingbing · 2018-07-26T03:58:50Z

@HyukjinKwon could you help to merge this to master branch? Thanks.

HyukjinKwon · 2018-07-26T04:15:23Z

Let me leave this open for few days in case some reviewers have more comments on this.

HyukjinKwon · 2018-07-27T05:27:12Z

Merged to master.

[SPARK-24829][SQL]CAST AS FLOAT inconsistent with Hive

6101770

xuanyuanking reviewed Jul 17, 2018

View reviewed changes

dongjoon-hyun reviewed Jul 17, 2018

View reviewed changes

zuotingbing changed the title ~~[SPARK-24829][SQL]CAST AS FLOAT inconsistent with Hive~~ [SPARK-24829][SQL]In Spark Thrift Server, CAST AS FLOAT inconsistent with spark-shell or spark-sql Jul 18, 2018

mgaido91 reviewed Jul 18, 2018

View reviewed changes

it is probably better to add test into HiveThriftJdbcTest.

ffcff14

zuotingbing changed the title ~~[SPARK-24829][SQL]In Spark Thrift Server, CAST AS FLOAT inconsistent with spark-shell or spark-sql~~ [SPARK-24829][STS]In Spark Thrift Server, CAST AS FLOAT inconsistent with spark-shell or spark-sql Jul 20, 2018

romove the duplicated test

05cbdc8

revert the code about "it is probably better to add test into HiveThr…

88dff75

…iftJdbcTest" which makes the UISeleniumSuite test failed

HyukjinKwon approved these changes Jul 25, 2018

View reviewed changes

asfgit closed this in dc3713c Jul 27, 2018

zuotingbing deleted the SPARK-24829 branch July 27, 2018 05:54

[SPARK-24829][STS]In Spark Thrift Server, CAST AS FLOAT inconsistent with spark-shell or spark-sql #21789

[SPARK-24829][STS]In Spark Thrift Server, CAST AS FLOAT inconsistent with spark-shell or spark-sql #21789

Conversation

zuotingbing commented Jul 17, 2018 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuotingbing Jul 19, 2018 • edited Loading

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

dongjoon-hyun left a comment

Choose a reason for hiding this comment

zuotingbing commented Jul 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuotingbing Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuotingbing Jul 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mgaido91 commented Jul 20, 2018

mgaido91 commented Jul 20, 2018

zuotingbing commented Jul 20, 2018 • edited Loading

HyukjinKwon commented Jul 21, 2018

SparkQA commented Jul 21, 2018

zuotingbing commented Jul 23, 2018 • edited Loading

HyukjinKwon commented Jul 23, 2018

SparkQA commented Jul 23, 2018

zuotingbing commented Jul 23, 2018 • edited Loading

mgaido91 commented Jul 23, 2018

HyukjinKwon commented Jul 23, 2018

HyukjinKwon commented Jul 23, 2018

SparkQA commented Jul 23, 2018

SparkQA commented Jul 23, 2018

zuotingbing commented Jul 24, 2018

HyukjinKwon commented Jul 24, 2018

SparkQA commented Jul 24, 2018

mgaido91 commented Jul 25, 2018

zuotingbing commented Jul 26, 2018

HyukjinKwon commented Jul 26, 2018

HyukjinKwon commented Jul 27, 2018

zuotingbing commented Jul 17, 2018 •

edited

Loading

zuotingbing Jul 19, 2018 •

edited

Loading

zuotingbing Jul 19, 2018 •

edited

Loading

zuotingbing Jul 19, 2018 •

edited

Loading

zuotingbing commented Jul 20, 2018 •

edited

Loading

zuotingbing commented Jul 23, 2018 •

edited

Loading

zuotingbing commented Jul 23, 2018 •

edited

Loading