Skip to content
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.

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

[SPARK-13332][SQL] Decimal datatype support for SQL pow #11212

Closed
wants to merge 3 commits into from

Conversation

yucai
Copy link
Contributor

@yucai yucai commented Feb 16, 2016

Decimal datatype support for SQL pow

  • when base is Decimal and exponent is integer(Byte, Short, Int), calculate and return as Decimal
  • otherwise, calculate and return as Double

https://issues.apache.org/jira/browse/SPARK-13332

@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@yucai
Copy link
Contributor Author

yucai commented Feb 16, 2016

@adrian-wang could you help review? Much thanks!

@@ -103,8 +103,7 @@ class MathFunctionsSuite extends SparkFunSuite with ExpressionEvalHelper {
}
} else {
domain.foreach { case (v1, v2) =>
checkEvaluation(c(Literal(v1), Literal(v2)), f(v1 + 0.0, v2 + 0.0), EmptyRow)
checkEvaluation(c(Literal(v2), Literal(v1)), f(v2 + 0.0, v1 + 0.0), EmptyRow)
checkEvaluation(c(Literal(v1), Literal(v2)), f(v1, v2), EmptyRow)
Copy link
Contributor

Choose a reason for hiding this comment

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

keep the test of f(v2, v1)

@yucai yucai changed the title [WIP][SQL] Decimal datatype support for pow [WIP][SPARK-13332][SQL] Decimal datatype support for SQL pow Feb 16, 2016
@@ -351,6 +350,20 @@ class MathFunctionsSuite extends SparkFunSuite with ExpressionEvalHelper {
}

test("pow") {
testBinary(Pow, (d: Decimal, n: Byte) => d.pow(n),
(-5 to 5).map(v => (Decimal(v * 1.0), v.toByte)))
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe v.toDouble is better

@rxin
Copy link
Contributor

rxin commented Feb 16, 2016

I think it'd be a lot simpler if we create a separate Pow for Decimal, and handle byte/short/etc to integer in type coercion, rather than in the PowDecimal class.

@yucai
Copy link
Contributor Author

yucai commented Feb 18, 2016

OK, let me try this solution.

@yucai
Copy link
Contributor Author

yucai commented Feb 19, 2016

@rxin I tried your suggestion like creating PowDecimal for Decimal specially like below:

case class PowDecimal(left: Expression, right: Expression)
  extends BinaryMathExpression(math.pow, "POWER") {
  override def inputTypes: Seq[AbstractDataType] = Seq(DecimalType, IntegerType)
  ...
}

case class Pow(left: Expression, right: Expression)
  extends BinaryMathExpression(math.pow, "POWER") {
  ...
} 

But one concern is when "select pow(cast(2 as decimal(5,2)), 3)", how to make the "PowDecimal" node created? The current path will create "Pow" node anyway.

So we think of maybe we can still put Decimal processing in "Pow", but byte/short/etc. to integer in type coercion, like below:

case class Pow(left: Expression, right: Expression)
  extends BinaryMathExpression(math.pow, "POWER") {
  override def inputTypes: Seq[AbstractDataType] = Seq(NumericType, NumericType)

  override def dataType: DataType = (left.dataType, right.dataType) match {
    case (dt: DecimalType, ByteType | ShortType | IntegerType) => dt
    case _ => DoubleType
  }
  protected override def nullSafeEval(input1: Any, input2: Any): Any =
    (left.dataType, right.dataType) match {
      case (dt: DecimalType, _) => input1.asInstanceOf[Decimal].pow(input2.asInstanceOf[Int])
      case _ => math.pow(input1.asInstanceOf[Double], input2.asInstanceOf[Double])
    }
  override def genCode(ctx: CodegenContext, ev: ExprCode): String = ...
}

In HiveTypeCoercion:

  object PowCoercion extends Rule[LogicalPlan] {
    def apply(plan: LogicalPlan): LogicalPlan = plan resolveExpressions {
      case e if !e.childrenResolved => e
      case e @ Pow(left, right) =>
        (left.dataType, right.dataType) match {
          case (dt: DecimalType, IntegerType) => e
          case (DoubleType, DoubleType) => e
          case (dt: DecimalType, ByteType | ShortType) => Pow(left, Cast(right, IntegerType))
          case (DoubleType, _) => Pow(left, Cast(right, DoubleType))
          case (_, DoubleType) => Pow(Cast(left, DoubleType), right)
          case _ => Pow(Cast(left, DoubleType), Cast(right, DoubleType))
        }
    }
  }

How do you think of this way?

@yucai yucai changed the title [WIP][SPARK-13332][SQL] Decimal datatype support for SQL pow [SPARK-13332][SQL] Decimal datatype support for SQL pow Feb 21, 2016
@rxin
Copy link
Contributor

rxin commented Jun 15, 2016

Thanks for the pull request. I'm going through a list of pull requests to cut them down since the sheer number is breaking some of the tooling we have. Due to lack of activity on this pull request, I'm going to push a commit to close it. Feel free to reopen it or create a new one. We can also continue the discussion on the JIRA ticket.

@asfgit asfgit closed this in 1a33f2e Jun 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants