-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-16323] [SQL] Add IntegerDivide to avoid unnecessary cast #14036
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
Changes from all commits
067b788
e4e42c3
5db17fd
faa2fd3
e30747a
699ff8b
054d54e
237a1ad
ff97457
f85433b
f35e605
e89ffc0
a4b39b3
2c02370
38f8bd4
a461e35
b2f4489
d057546
01f5ed2
feee3cd
ab6858c
8d9a04d
7e4e394
7bb3e43
1ba502f
16eff20
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,20 +207,12 @@ case class Multiply(left: Expression, right: Expression) | |
| protected override def nullSafeEval(input1: Any, input2: Any): Any = numeric.times(input1, input2) | ||
| } | ||
|
|
||
| @ExpressionDescription( | ||
| usage = "a _FUNC_ b - Divides a by b.", | ||
| extended = "> SELECT 3 _FUNC_ 2;\n 1.5") | ||
| case class Divide(left: Expression, right: Expression) | ||
| extends BinaryArithmetic with NullIntolerant { | ||
|
|
||
| override def inputType: AbstractDataType = TypeCollection(DoubleType, DecimalType) | ||
|
|
||
| override def symbol: String = "/" | ||
| override def decimalMethod: String = "$div" | ||
| abstract class DivideBase extends BinaryArithmetic with NullIntolerant { | ||
| override def nullable: Boolean = true | ||
|
|
||
| private lazy val div: (Any, Any) => Any = dataType match { | ||
| case ft: FractionalType => ft.fractional.asInstanceOf[Fractional[Any]].div | ||
| case i: IntegralType => i.integral.asInstanceOf[Integral[Any]].quot | ||
| } | ||
|
|
||
| override def eval(input: InternalRow): Any = { | ||
|
|
@@ -250,10 +242,11 @@ case class Divide(left: Expression, right: Expression) | |
| } | ||
| val javaType = ctx.javaType(dataType) | ||
| val divide = if (dataType.isInstanceOf[DecimalType]) { | ||
| s"${eval1.value}.$decimalMethod(${eval2.value})" | ||
| s"${eval1.value}.$$div(${eval2.value})" | ||
| } else { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just keep it? Then the 2 implementations can share the same codegen. |
||
| s"($javaType)(${eval1.value} $symbol ${eval2.value})" | ||
| s"($javaType)(${eval1.value} / ${eval2.value})" | ||
| } | ||
|
|
||
| if (!left.nullable && !right.nullable) { | ||
| ev.copy(code = s""" | ||
| ${eval2.code} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it already cover both fraction and integral division? |
||
|
|
@@ -284,6 +277,26 @@ case class Divide(left: Expression, right: Expression) | |
| } | ||
| } | ||
|
|
||
| @ExpressionDescription( | ||
| usage = "a _FUNC_ b - Fraction Division a by b.", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about |
||
| extended = "> SELECT 3 _FUNC_ 2;\n 1.5") | ||
| case class Divide(left: Expression, right: Expression) extends DivideBase { | ||
|
|
||
| override def inputType: AbstractDataType = TypeCollection(DoubleType, DecimalType) | ||
|
|
||
| override def symbol: String = "/" | ||
| } | ||
|
|
||
| @ExpressionDescription( | ||
| usage = "a _FUNC_ b - Divides a by b.", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. how about |
||
| extended = "> SELECT 3 _FUNC_ 2;\n 1") | ||
| case class IntegralDivide(left: Expression, right: Expression) extends DivideBase { | ||
|
|
||
| override def inputType: AbstractDataType = IntegralType | ||
|
|
||
| override def symbol: String = "div" | ||
| } | ||
|
|
||
| @ExpressionDescription( | ||
| usage = "a _FUNC_ b - Returns the remainder when dividing a by b.") | ||
| case class Remainder(left: Expression, right: Expression) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -957,7 +957,7 @@ class AstBuilder extends SqlBaseBaseVisitor[AnyRef] with Logging { | |
| case SqlBaseParser.PERCENT => | ||
| Remainder(left, right) | ||
| case SqlBaseParser.DIV => | ||
| Cast(Divide(left, right), LongType) | ||
| IntegralDivide(left, right) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to add SqlBaseParser.DIVIDE for '/'. BTW: SqlBaseParser.DIV for 'div' .
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's ok because I find SparkSQL has SqlBaseParser.SLASH for '/' . |
||
| case SqlBaseParser.PLUS => | ||
| Add(left, right) | ||
| case SqlBaseParser.MINUS => | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cloud-fan how about make this line in IntegralDivide that can be more readable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
DivideBaseimplement codegen for all types, so I think it's fine for it to implementevalfor all types.