Skip to content

[SPARK-25454][SQL] add a new config for picking minimum precision for integral literals #22494

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cloud-fan wants to merge 3 commits into from
Closed

[SPARK-25454][SQL] add a new config for picking minimum precision for integral literals #22494

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad79c56
add a config to turn off precise precision for integral literals
cloud-fan Sep 20, 2018
b4fdd13
address comments
cloud-fan Sep 21, 2018
faa0bd0
add legacy
cloud-fan Sep 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 6 additions & 4 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/DecimalPrecision.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,11 +290,13 @@ object DecimalPrecision extends TypeCoercionRule {
// potentially loosing 11 digits of the fractional part. Using only the precision needed
// by the Literal, instead, the result would be DECIMAL(38 + 1 + 1, 18), which would
// become DECIMAL(38, 16), safely having a much lower precision loss.
case (l: Literal, r) if r.dataType.isInstanceOf[DecimalType]
&& l.dataType.isInstanceOf[IntegralType] =>
case (l: Literal, r) if r.dataType.isInstanceOf[DecimalType] &&
l.dataType.isInstanceOf[IntegralType] &&
SQLConf.get.literalPrecisePrecision =>
b.makeCopy(Array(Cast(l, DecimalType.fromLiteral(l)), r))
case (l, r: Literal) if l.dataType.isInstanceOf[DecimalType]
&& r.dataType.isInstanceOf[IntegralType] =>
case (l, r: Literal) if l.dataType.isInstanceOf[DecimalType] &&
r.dataType.isInstanceOf[IntegralType] &&
SQLConf.get.literalPrecisePrecision =>
b.makeCopy(Array(l, Cast(r, DecimalType.fromLiteral(r))))
// Promote integers inside a binary expression with fixed-precision decimals to decimals,
// and fixed-precision decimals in an expression with floats / doubles to doubles
Expand Down
12 changes: 12 additions & 0 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1345,6 +1345,16 @@ object SQLConf {
.booleanConf
.createWithDefault(true)

val LITERAL_PRECISE_PRECISION =
Copy link
Contributor

@mgaido91 mgaido91 Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mmh... PRECISE_PRECISION sounds weird... can we look for a better one? I don't have a suggestion right now, but I'll think about it.

buildConf("spark.sql.literal.precisePrecision")
.internal()
.doc("When integral literals are used with decimals in binary operators, Spark will " +
"pick a precise precision for the literals to calculate the precision and scale " +
Copy link
Contributor

@mgaido91 mgaido91 Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a precise precision -> the minimal precision required to represent the given value?

"of the result decimal, when this config is true. By picking a precise precision, we " +
"can avoid wasting precision, to reduce the possibility of overflow.")
Copy link
Contributor

@mgaido91 mgaido91 Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to reduce the possibility of overflow actually this is not true and depends on the value of DECIMAL_OPERATIONS_ALLOW_PREC_LOSS, If DECIMAL_OPERATIONS_ALLOW_PREC_LOSS is true, the risk is to have a precision loss, but we don't overflow. If that is false, then this statement is right.

.booleanConf
.createWithDefault(true)

val SQL_OPTIONS_REDACTION_PATTERN =
buildConf("spark.sql.redaction.options.regex")
.doc("Regex to decide which keys in a Spark SQL command's options map contain sensitive " +
Expand Down Expand Up @@ -1939,6 +1949,8 @@ class SQLConf extends Serializable with Logging {

def decimalOperationsAllowPrecisionLoss: Boolean = getConf(DECIMAL_OPERATIONS_ALLOW_PREC_LOSS)

def literalPrecisePrecision: Boolean = getConf(LITERAL_PRECISE_PRECISION)

def continuousStreamingExecutorQueueSize: Int = getConf(CONTINUOUS_STREAMING_EXECUTOR_QUEUE_SIZE)

def continuousStreamingExecutorPollIntervalMs: Long =
Expand Down
7 changes: 7 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2858,6 +2858,13 @@ class SQLQuerySuite extends QueryTest with SharedSQLContext {
val result = ds.flatMap(_.bar).distinct
result.rdd.isEmpty
}

test("SPARK-25454: decimal division with negative scale") {
// TODO: completely fix this issue even LITERAL_PRECISE_PRECISION is true.
Copy link
Contributor

@mgaido91 mgaido91 Sep 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: even when

withSQLConf(SQLConf.LITERAL_PRECISE_PRECISION.key -> "false") {
checkAnswer(sql("select 26393499451 / (1e6 * 1000)"), Row(BigDecimal("26.3934994510000")))
}
}
}

case class Foo(bar: Option[String])