You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by cloud-fan <gi...@git.apache.org> on 2018/09/19 16:25:54 UTC
[GitHub] spark pull request #22470: [SPARK-25454][SQL] should not generate negative s...
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/22470
[SPARK-25454][SQL] should not generate negative scale as possible as we can
## What changes were proposed in this pull request?
An alternative fix of https://github.com/apache/spark/pull/22450
The issue is, Spark SQL accepts negative decimal scale by accident (I believe so), and have problems to deal with negative scale for some cases (e.g. decimal division, [decimal rounding](https://community.oracle.com/thread/966388)).
The best solution is to officially support negative scale and make the behaviors following SQL standard, but this is hard to do and will take a long time. Or we can officially forbid negative scale, which is a breaking change and we can't do it as a bug fix.
This PR proposes to avoid generating negative scale as possible as we can, to work around this issue for common cases.
## How was this patch tested?
new tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark decimal
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22470.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #22470
----
commit 3c05636b879b678cfcf72dfb515ea691317e470d
Author: Wenchen Fan <we...@...>
Date: 2018-09-19T15:36:49Z
should not generate negative scale as possible as we can
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22470
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/testing-k8s-prb-make-spark-distribution-unified/3251/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/22470
@mgaido91 makes sense. Actually @cloud-fan had asked me to write some test cases for decimal values with -ve scale in another PR. While i was playing around, i found this issue. It seemed to me that the whole -ve scale thingy is not thought through properly. Thats the reason i wanted to share my findings here while you guys are discussing this :-).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/22470
@cloud-fan I think the main problem about this (and it is the reason why I haven't proposed it) is that the range of operations supported would be smaller, so we may forbid operations which now can happen. For instance, the following code has been always working on Spark: `lit(BigDecimal(1e36)) * lit(BigDecimal(10000))`. Indeed now this would become a `decimal(6, -36)`. With your change, this is going to be a `decimal(42, 0)` which is out of the range of supported values (ie. an overflow would occur).
I am not sure if any user has something like this, but it is possible and I think we cannot exclude it. We may, though, restrict again the condition when it happens, ie. in case we are just parsing from a literal we can avoid returning a negative scale. But the other fix would be needed anyway in this case, as we could still have to deal with negative scales, so IMO this would be quite useless.
I'd agree though about forbidding negative scale in 3.0.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/22470
@cloud-fan Could you please check CSVinferSchema::tryParseDecimal() ? There is a condition to check negative scale.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22470
cc @mgaido91 @hvanhovell @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22470
**[Test build #96271 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96271/testReport)** for PR 22470 at commit [`3c05636`](https://github.com/apache/spark/commit/3c05636b879b678cfcf72dfb515ea691317e470d).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22470
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22470
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22470
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/96271/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22470: [SPARK-25454][SQL] should not generate negative s...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan closed the pull request at:
https://github.com/apache/spark/pull/22470
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22470
**[Test build #96271 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/96271/testReport)** for PR 22470 at commit [`3c05636`](https://github.com/apache/spark/commit/3c05636b879b678cfcf72dfb515ea691317e470d).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by dilipbiswal <gi...@git.apache.org>.
Github user dilipbiswal commented on the issue:
https://github.com/apache/spark/pull/22470
@mgaido91 @cloud-fan On the other hand .. some use cases may work better :-) , for example
Before
```
scala> spark.sql("create table dec as select (1e36 * 1) as col1")
org.apache.spark.SparkException: Cannot recognize hive type string: decimal(3,-36)
at org.apache.spark.sql.hive.client.HiveClientImpl$.org$apache$spark$sql$hive$client$HiveClientImpl$$getSparkSQLDataType(HiveClientImpl.scala:883)
at org.apache.spark.sql.hive.client.HiveClientImpl$$anonfun$org$apache$spark$sql$hive$client$HiveClientImpl$$verifyColumnDataType$1.apply(HiveClientImpl.scala:905)
````
with this pr
```
scala> spark.sql("create table dec as select (1e36 * 1) as col1")
18/09/19 14:29:29 WARN HiveMetaStore: Location: file:/user/hive/warehouse/dec specified for non-external table:dec
18/09/19 14:29:30 WARN ObjectStore: Failed to get database global_temp, returning NoSuchObjectException
res0: org.apache.spark.sql.DataFrame = []
scala> spark.sql("describe table dec").show
+--------+-------------+-------+
|col_name| data_type|comment|
+--------+-------------+-------+
| col1|decimal(38,0)| null|
+--------+-------------+-------+
```
Perhaps we may have issues writing out data frames containing decimal with negative scale to file based datasources as well. I have not verified though.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by mgaido91 <gi...@git.apache.org>.
Github user mgaido91 commented on the issue:
https://github.com/apache/spark/pull/22470
@dilipbiswal yes, I definitely think that in general we should get to forbid negative scales. I only thought that this should be done in 3.0 rather than now. And for now the safest option to me was to update properly the only rule which was not working correctly in that case. I also agree in avoiding negative scales in places where they were not possible before 2.3, ie. when parsing from literals. But as there may be other cases where this happens, this solves only part of the problem.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22470: [SPARK-25454][SQL] should not generate negative scale as...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/22470
@mgaido91 you are right, this still has behavior changes if the intermedia result exceed the max precision. Since most of the storages don't support negative scale(hive, parquet, etc.), I think we should eventually forbid it too. Let's move the discussion to #22450
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org