Skip to content

[SPARK-16735][SQL] map should create a decimal key or value from decimals with different precisions and scales #14374

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
biglobster wants to merge 6 commits into from
Closed

[SPARK-16735][SQL] map should create a decimal key or value from decimals with different precisions and scales #14374

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c9651fd
Summary: Fail to create a map contains decimal type with literals hav…
biglobster Jul 26, 2016
ba2560e
Summary:Fail to create a map contains decimal type with literals havi…
biglobster Jul 26, 2016
3fa2153
Summary: Fail to create a map contains decimal type with literals hav…
biglobster Jul 26, 2016
7143ed6
Summary:fix jira_id in the test("SPARK-16735: CreateMap with Decimals")
biglobster Jul 26, 2016
86b1171
Summary:change the isTighterThan to isDecimalTypeTighterThan
biglobster Jul 28, 2016
c064621
Summary:add "spark.sql.broadcastTimeout" into docs/sql-programming-gu…
biglobster Aug 3, 2016
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
37 changes: 31 additions & 6 deletions ...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,20 +93,45 @@ case class CreateMap(children: Seq[Expression]) extends Expression {
if (children.size % 2 != 0) {
TypeCheckResult.TypeCheckFailure(s"$prettyName expects a positive even number of arguments.")
} else if (keys.map(_.dataType).distinct.length > 1) {
TypeCheckResult.TypeCheckFailure("The given keys of function map should all be the same " +
"type, but they are " + keys.map(_.dataType.simpleString).mkString("[", ", ", "]"))
if (keys.map(_.dataType).forall(_.isInstanceOf[DecimalType])) {
TypeCheckResult.TypeCheckSuccess
} else {
TypeCheckResult.TypeCheckFailure("The given keys of function map should all be the same " +
"type, but they are " + keys.map(_.dataType.simpleString).mkString("[", ", ", "]"))
}
} else if (values.map(_.dataType).distinct.length > 1) {
TypeCheckResult.TypeCheckFailure("The given values of function map should all be the same " +
"type, but they are " + values.map(_.dataType.simpleString).mkString("[", ", ", "]"))
if (values.map(_.dataType).forall(_.isInstanceOf[DecimalType])) {
TypeCheckResult.TypeCheckSuccess
} else {
TypeCheckResult.TypeCheckFailure("The given values of function map should all be the " +
"same type, but they are " + values.map(_.dataType.simpleString).mkString("[", ", ", "]"))
}
} else {
TypeCheckResult.TypeCheckSuccess
}
}

private def checkDecimalType(colType: Seq[Expression]): DataType = {
val elementType = colType.headOption.map(_.dataType).getOrElse(NullType)
elementType match {
case _ if elementType.isInstanceOf[DecimalType] =>
var tighter: DataType = elementType
colType.foreach { child =>
if (elementType.asInstanceOf[DecimalType].isTighterThan(child.dataType)) {

@rxin rxin Jul 27, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

isTighterThan is not associative - i think this would be a problem?

@biglobster biglobster Jul 27, 2016 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@rxin I have checked this function, and it will not lost any precision or range ,it 's safe .
and in the checkDecimalType, we just check the datatype and do not change datatype.
(when keys or values contains integer type , it will pass. but still integer type)
so checkInputDataTypes will return result like it done before.
and InCase when keys or values contains integer type, I will use a new function instead of isTighterThan that do not check integer type.
can you give me some advise ? thank you :)

@rxin rxin Jul 28, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I was referring to was that isTighterThan was not associative, and i don't think you can just take the tightest one this way.

As an example:

a precision 10, scale 5
b precision 7, scale 1

in this case a is not tighter than b, but b would be chosen as the target data type, leading to lose of precision.

@biglobster biglobster Jul 28, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

thx：） get it

tighter = child.dataType
}
}

tighter
case _ =>
elementType
}
}

override def dataType: DataType = {
MapType(
keyType = keys.headOption.map(_.dataType).getOrElse(NullType),
valueType = values.headOption.map(_.dataType).getOrElse(NullType),
keyType = checkDecimalType(keys),
valueType = checkDecimalType(values),
valueContainsNull = values.exists(_.nullable))
}

Expand Down
31 changes: 22 additions & 9 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
package org.apache.spark.sql.catalyst.expressions

import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.analysis.UnresolvedExtractValue
import org.apache.spark.sql.catalyst.analysis.{TypeCheckResult, UnresolvedExtractValue}
import org.apache.spark.sql.catalyst.dsl.expressions._
import org.apache.spark.sql.types._
import org.apache.spark.unsafe.types.UTF8String
Expand Down Expand Up @@ -134,16 +134,29 @@ class ComplexTypeSuite extends SparkFunSuite with ExpressionEvalHelper {
checkEvaluation(CreateArray(Literal.create(null, IntegerType) :: Nil), null :: Nil)
}

test("CreateMap") {
def interlace(keys: Seq[Literal], values: Seq[Literal]): Seq[Literal] = {
keys.zip(values).flatMap { case (k, v) => Seq(k, v) }
}
private def interlace(keys: Seq[Literal], values: Seq[Literal]): Seq[Literal] = {
keys.zip(values).flatMap { case (k, v) => Seq(k, v) }
}

def createMap(keys: Seq[Any], values: Seq[Any]): Map[Any, Any] = {
// catalyst map is order-sensitive, so we create ListMap here to preserve the elements order.
scala.collection.immutable.ListMap(keys.zip(values): _*)
}
private def createMap(keys: Seq[Any], values: Seq[Any]): Map[Any, Any] = {
// catalyst map is order-sensitive, so we create ListMap here to preserve the elements order.
scala.collection.immutable.ListMap(keys.zip(values): _*)
}

test("SPARK-16735: CreateMap with Decimals") {
val keys = Seq(0.02, 0.004)
val values = Seq(0.001, 0.5)
val keys1 = Seq(0.020, 0.004)
val values1 = Seq(0.001, 0.500)
val map1 = CreateMap(interlace(keys.map(Literal(_)), values.map(Literal(_))))

assert(map1.checkInputDataTypes() == TypeCheckResult.TypeCheckSuccess)

checkEvaluation(map1, createMap(keys1, values1))
checkEvaluation(map1, createMap(keys, values))
}

test("CreateMap") {
val intSeq = Seq(5, 10, 15, 20, 25)
val longSeq = intSeq.map(_.toLong)
val strSeq = intSeq.map(_.toString)
Expand Down