[GitHub] [incubator-livy] mgaido91 commented on a change in pull request #296: [LIVY-771][THRIFT] Do not remove trailing zeros from decimal values.
mgaido91 commented on a change in pull request #296:
URL: https://github.com/apache/incubator-livy/pull/296#discussion_r451698726
##
File path:
thriftserver/session/src/main/java/org/apache/livy/thriftserver/session/ResultSet.java
##
@@ -88,7 +88,7 @@ private String toHiveString(Object value, boolean
quoteStrings) {
} else if (quoteStrings && value instanceof String) {
return "\"" + value + "\"";
} else if (value instanceof BigDecimal) {
- return ((BigDecimal) value).stripTrailingZeros().toString();
+ return ((BigDecimal) value).toString();
Review comment:
yes, the problem is Livy support Spark 2.4 currently... I can just
repeat what I said earlier: adding such a test can just bring a benefit and no
harm. If you think that adding this test can be harmful for Livy, please just
state why, otherwise I think we can add the test and go ahead with this PR.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-livy] mgaido91 commented on a change in pull request #296: [LIVY-771][THRIFT] Do not remove trailing zeros from decimal values.
mgaido91 commented on a change in pull request #296:
URL: https://github.com/apache/incubator-livy/pull/296#discussion_r446423157
##
File path:
thriftserver/session/src/main/java/org/apache/livy/thriftserver/session/ResultSet.java
##
@@ -88,7 +88,7 @@ private String toHiveString(Object value, boolean
quoteStrings) {
} else if (quoteStrings && value instanceof String) {
return "\"" + value + "\"";
} else if (value instanceof BigDecimal) {
- return ((BigDecimal) value).stripTrailingZeros().toString();
+ return ((BigDecimal) value).toString();
Review comment:
I agree it is a corner case, but I think tests exist also for corner
cases, you don't want a software which behaves poorly even in corner cases.
Moreover I think it is not a big issue to just add a test for this case, which
if/when Spark won't support at all that case anymore can be removed. I see no
harm in adding a test and a benefit.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-livy] mgaido91 commented on a change in pull request #296: [LIVY-771][THRIFT] Do not remove trailing zeros from decimal values.
mgaido91 commented on a change in pull request #296:
URL: https://github.com/apache/incubator-livy/pull/296#discussion_r443259717
##
File path:
thriftserver/session/src/main/java/org/apache/livy/thriftserver/session/ResultSet.java
##
@@ -88,7 +88,7 @@ private String toHiveString(Object value, boolean
quoteStrings) {
} else if (quoteStrings && value instanceof String) {
return "\"" + value + "\"";
} else if (value instanceof BigDecimal) {
- return ((BigDecimal) value).stripTrailingZeros().toString();
+ return ((BigDecimal) value).toString();
Review comment:
with Livy thriftserver, you can run SQL queries on an existing Spark
session. You can, for instance, create a Livy scala session, run the first two
commands of my example, then create a Livy thriftsession which re-uses the
scala session (by specifying its `sessionId` when connecting) and run the SQL
query from my example.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-livy] mgaido91 commented on a change in pull request #296: [LIVY-771][THRIFT] Do not remove trailing zeros from decimal values.
mgaido91 commented on a change in pull request #296:
URL: https://github.com/apache/incubator-livy/pull/296#discussion_r437230996
##
File path:
thriftserver/session/src/main/java/org/apache/livy/thriftserver/session/ResultSet.java
##
@@ -88,7 +88,7 @@ private String toHiveString(Object value, boolean
quoteStrings) {
} else if (quoteStrings && value instanceof String) {
return "\"" + value + "\"";
} else if (value instanceof BigDecimal) {
- return ((BigDecimal) value).stripTrailingZeros().toString();
+ return ((BigDecimal) value).toString();
Review comment:
The SQL parser does not allow it, indeed, but this doesn't mean you
cannot create a table with negative scales and then query it with Spark SQL, as
I have shown above. I think it is important to enforce the behavior we have in
that case too and the correct behavior should be what SparkSQL shows IMHO. So
please do add a test case.
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-livy] mgaido91 commented on a change in pull request #296: [LIVY-771][THRIFT] Do not remove trailing zeros from decimal values.
mgaido91 commented on a change in pull request #296:
URL: https://github.com/apache/incubator-livy/pull/296#discussion_r429547231
##
File path:
thriftserver/session/src/main/java/org/apache/livy/thriftserver/session/ResultSet.java
##
@@ -88,7 +88,7 @@ private String toHiveString(Object value, boolean
quoteStrings) {
} else if (quoteStrings && value instanceof String) {
return "\"" + value + "\"";
} else if (value instanceof BigDecimal) {
- return ((BigDecimal) value).stripTrailingZeros().toString();
+ return ((BigDecimal) value).toString();
Review comment:
It does make sense, as in Spark, which we use, negative scales are
definitely allowed. So we must define a behavior, comparing us with Spark and
Hive (if it supports it) for these cases too.
Just to give you a reproducer, you can create a table with a negative scale
very easily with Spark and query it through SQL like this:
```
scala> val df = spark.range(10).select($"id".cast(DecimalType(10,-1)))
df: org.apache.spark.sql.DataFrame = [id: decimal(10,-1)]
scala> df.show
++
| id|
++
|0E+1|
|0E+1|
|0E+1|
|0E+1|
|0E+1|
|1E+1|
|1E+1|
|1E+1|
|1E+1|
|1E+1|
++
scala> df.createTempView("aa")
scala> sql("select * from aa")
res3: org.apache.spark.sql.DataFrame = [id: decimal(10,-1)]
scala> sql("select * from aa").show
++
| id|
++
|0E+1|
|0E+1|
|0E+1|
|0E+1|
|0E+1|
|1E+1|
|1E+1|
|1E+1|
|1E+1|
|1E+1|
++
```
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [incubator-livy] mgaido91 commented on a change in pull request #296: [LIVY-771][THRIFT] Do not remove trailing zeros from decimal values.
mgaido91 commented on a change in pull request #296:
URL: https://github.com/apache/incubator-livy/pull/296#discussion_r429289089
##
File path:
thriftserver/session/src/main/java/org/apache/livy/thriftserver/session/ResultSet.java
##
@@ -88,7 +88,7 @@ private String toHiveString(Object value, boolean
quoteStrings) {
} else if (quoteStrings && value instanceof String) {
return "\"" + value + "\"";
} else if (value instanceof BigDecimal) {
- return ((BigDecimal) value).stripTrailingZeros().toString();
+ return ((BigDecimal) value).toString();
Review comment:
to be coherent with Hive, shall we use `toPlainString`? can we also add
a test with negative scale for this case? can you also check Spark's behavior
in this case?
##
File path:
thriftserver/session/src/test/java/org/apache/livy/thriftserver/session/ColumnBufferTest.java
##
@@ -76,6 +76,8 @@ public void testColumnBuffer() throws Exception {
Encoder encoder = Encoders.bean(TestBean.class);
Dataset ds = spark.createDataset(Arrays.asList(tb), encoder);
+ // In ds, the decimal field will be a
org.apache.spark.sql.types.DecimalType,
Review comment:
a weird place for a comment... don't understand why to put it here...
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
