Skip to content
Closed
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
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
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ object FunctionRegistry {
expression[Subtract]("-"),
expression[Multiply]("*"),
expression[Divide]("/"),
expression[IntegerDivide]("div"),
Copy link
Contributor

Choose a reason for hiding this comment

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

does hive support this syntax? i.e. div(4, 2)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so.

Copy link
Contributor

Choose a reason for hiding this comment

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

@cloud-fan yes, hive support div and / .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lianhuiwang doing div(4,2) gives

hive> div(4, 2);
NoViableAltException(14@[])
    at org.apache.hadoop.hive.ql.parse.HiveParser.statement(HiveParser.java:1099)
    at org.apache.hadoop.hive.ql.parse.ParseDriver.parse(ParseDriver.java:204)
    at org.apache.hadoop.hive.ql.parse.ParseDriver.parse(ParseDriver.java:166)
    at org.apache.hadoop.hive.ql.Driver.compile(Driver.java:440)
    at org.apache.hadoop.hive.ql.Driver.compile(Driver.java:319)
    at org.apache.hadoop.hive.ql.Driver.compileInternal(Driver.java:1249)
    at org.apache.hadoop.hive.ql.Driver.runInternal(Driver.java:1295)
    at org.apache.hadoop.hive.ql.Driver.run(Driver.java:1178)
    at org.apache.hadoop.hive.ql.Driver.run(Driver.java:1166)
    at org.apache.hadoop.hive.cli.CliDriver.processLocalCmd(CliDriver.java:236)
    at org.apache.hadoop.hive.cli.CliDriver.processCmd(CliDriver.java:187)
    at org.apache.hadoop.hive.cli.CliDriver.processLine(CliDriver.java:403)
    at org.apache.hadoop.hive.cli.CliDriver.executeDriver(CliDriver.java:782)
    at org.apache.hadoop.hive.cli.CliDriver.run(CliDriver.java:721)
    at org.apache.hadoop.hive.cli.CliDriver.main(CliDriver.java:648)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:497)
    at org.apache.hadoop.util.RunJar.run(RunJar.java:221)
    at org.apache.hadoop.util.RunJar.main(RunJar.java:136)
FAILED: ParseException line 1:0 cannot recognize input near 'div' '(' '4'

Copy link
Contributor

Choose a reason for hiding this comment

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

'select 4 div 2' is the right code.

expression[Remainder]("%"),

// aggregate functions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ package object dsl {
def - (other: Expression): Expression = Subtract(expr, other)
def * (other: Expression): Expression = Multiply(expr, other)
def / (other: Expression): Expression = Divide(expr, other)
def div (other: Expression): Expression = IntegerDivide(expr, other)
def % (other: Expression): Expression = Remainder(expr, other)
def & (other: Expression): Expression = BitwiseAnd(expr, other)
def | (other: Expression): Expression = BitwiseOr(expr, other)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 DivisionArithmetic extends BinaryArithmetic with NullIntolerant {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about DivideBase?

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
Copy link
Contributor

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

The DivideBase implement codegen for all types, so I think it's fine for it to implement eval for all types.

}

override def eval(input: InternalRow): Any = {
Expand All @@ -237,6 +229,9 @@ case class Divide(left: Expression, right: Expression)
}
}

// Used by doGenCode
protected def divide(eval1: ExprCode, eval2: ExprCode, javaType: String): String
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need this abstraction. this one already covers both fraction and integral

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did it on purpose. we can't call $div on byte's and plus if I try to call value = value1 / value2; for decimals, I get Binary numeric promotion not possible on types "org.apache.spark.sql.types.Decimal" and "org.apache.spark.sql.types.Decimal".

Copy link
Contributor

@cloud-fan cloud-fan Jul 12, 2016

Choose a reason for hiding this comment

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

you can see from this test that the previous Divide expression can support all integral types and fraction types.

BTW looks like there is no special handling for integral type in your PR, for fraction type:

if (dataType.isInstanceOf[DecimalType]) {
  s"${eval1.value}.$decimalMethod(${eval2.value})"
} else {
  s"($javaType)(${eval1.value} $symbol ${eval2.value})"
}

while the symbol is /, so we generate ($javaType)(${eval1.value} / ${eval2.value}) for double and float.

for integral type:

($javaType)(${eval1.value} $decimalMethod (${eval2.value}))

while the decimalMethod is /, so it still generates ($javaType)(${eval1.value} / ${eval2.value})

Copy link
Contributor Author

@techaddict techaddict Jul 12, 2016

Choose a reason for hiding this comment

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

@cloud-fan yes but getting A method named "$div" is not declared in any enclosing class nor any supertype, nor through a static import in the updated pr for Code generation of (2.0 / 1.0)


/**
* Special case handling due to division by 0 => null.
*/
Expand All @@ -249,11 +244,7 @@ case class Divide(left: Expression, right: Expression)
s"${eval2.value} == 0"
}
val javaType = ctx.javaType(dataType)
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

val divide = if (dataType.isInstanceOf[DecimalType]) {
s"${eval1.value}.$decimalMethod(${eval2.value})"
} else {
s"($javaType)(${eval1.value} $symbol ${eval2.value})"
}
val division = divide(eval1, eval2, javaType)
if (!left.nullable && !right.nullable) {
ev.copy(code = s"""
${eval2.code}
Expand All @@ -263,7 +254,7 @@ case class Divide(left: Expression, right: Expression)
${ev.isNull} = true;
} else {
${eval1.code}
${ev.value} = $divide;
${ev.value} = $division;
}""")
} else {
ev.copy(code = s"""
Expand All @@ -277,13 +268,51 @@ case class Divide(left: Expression, right: Expression)
if (${eval1.isNull}) {
${ev.isNull} = true;
} else {
${ev.value} = $divide;
${ev.value} = $division;
}
}""")
}
}
}

@ExpressionDescription(
usage = "a _FUNC_ b - Divides a by b.",
Copy link
Contributor

Choose a reason for hiding this comment

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

we should mention this is a fraction division, i.e. the parameter must be fraction type and the result is also fraction.

extended = "> SELECT 3 _FUNC_ 2;\n 1.5")
case class Divide(left: Expression, right: Expression)
extends DivisionArithmetic {

override def inputType: AbstractDataType = TypeCollection(DoubleType, DecimalType)

override def symbol: String = "/"
override def decimalMethod: String = "$div"

// Used by doGenCode
protected override def divide(eval1: ExprCode, eval2: ExprCode, javaType: String): String = {
if (dataType.isInstanceOf[DecimalType]) {
s"${eval1.value}.$decimalMethod(${eval2.value})"
} else {
s"($javaType)(${eval1.value} $symbol ${eval2.value})"
}
}
}

@ExpressionDescription(
usage = "a _FUNC_ b - Divides a by b.",
Copy link
Contributor

@cloud-fan cloud-fan Jul 13, 2016

Choose a reason for hiding this comment

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

how about Divides a by b of integral type?

extended = "> SELECT 3 _FUNC_ 2;\n 1")
case class IntegerDivide(left: Expression, right: Expression)
Copy link
Contributor

Choose a reason for hiding this comment

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

IntegralDivide?

extends DivisionArithmetic {

override def inputType: AbstractDataType = IntegralType

override def symbol: String = "div"
override def decimalMethod: String = "/"

// Used by doGenCode
protected override def divide(eval1: ExprCode, eval2: ExprCode, javaType: String): String = {
s"($javaType)(${eval1.value} $decimalMethod (${eval2.value}))"
}
}

@ExpressionDescription(
usage = "a _FUNC_ b - Returns the remainder when dividing a by b.")
case class Remainder(left: Expression, right: Expression)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
IntegerDivide(left, right)
case SqlBaseParser.PLUS =>
Add(left, right)
case SqlBaseParser.MINUS =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,14 +140,14 @@ class ArithmeticExpressionSuite extends SparkFunSuite with ExpressionEvalHelper

// By fixing SPARK-15776, Divide's inputType is required to be DoubleType of DecimalType.
// TODO: in future release, we should add a IntegerDivide to support integral types.
ignore("/ (Divide) for integral type") {
checkEvaluation(Divide(Literal(1.toByte), Literal(2.toByte)), 0.toByte)
checkEvaluation(Divide(Literal(1.toShort), Literal(2.toShort)), 0.toShort)
checkEvaluation(Divide(Literal(1), Literal(2)), 0)
checkEvaluation(Divide(Literal(1.toLong), Literal(2.toLong)), 0.toLong)
checkEvaluation(Divide(positiveShortLit, negativeShortLit), 0.toShort)
checkEvaluation(Divide(positiveIntLit, negativeIntLit), 0)
checkEvaluation(Divide(positiveLongLit, negativeLongLit), 0L)
test("/ (Divide) for integral type") {
checkEvaluation(IntegerDivide(Literal(1.toByte), Literal(2.toByte)), 0.toByte)
checkEvaluation(IntegerDivide(Literal(1.toShort), Literal(2.toShort)), 0.toShort)
checkEvaluation(IntegerDivide(Literal(1), Literal(2)), 0)
checkEvaluation(IntegerDivide(Literal(1.toLong), Literal(2.toLong)), 0.toLong)
checkEvaluation(IntegerDivide(positiveShortLit, negativeShortLit), 0.toShort)
checkEvaluation(IntegerDivide(positiveIntLit, negativeIntLit), 0)
checkEvaluation(IntegerDivide(positiveLongLit, negativeLongLit), 0L)
}

test("% (Remainder)") {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ class ExpressionParserSuite extends PlanTest {
// Simple operations
assertEqual("a * b", 'a * 'b)
assertEqual("a / b", 'a / 'b)
assertEqual("a DIV b", ('a / 'b).cast(LongType))
assertEqual("a DIV b", ('a div 'b))
assertEqual("a % b", 'a % 'b)
assertEqual("a + b", 'a + 'b)
assertEqual("a - b", 'a - 'b)
Expand All @@ -180,7 +180,7 @@ class ExpressionParserSuite extends PlanTest {
// Check precedences
assertEqual(
"a * t | b ^ c & d - e + f % g DIV h / i * k",
'a * 't | ('b ^ ('c & ('d - 'e + (('f % 'g / 'h).cast(LongType) / 'i * 'k)))))
'a * 't | ('b ^ ('c & ('d - 'e + (('f % 'g div 'h) / 'i * 'k)))))
}

test("unary arithmetic expressions") {
Expand Down
15 changes: 15 additions & 0 deletions sql/core/src/main/scala/org/apache/spark/sql/Column.scala
Original file line number Diff line number Diff line change
Expand Up @@ -731,6 +731,21 @@ class Column(protected[sql] val expr: Expression) extends Logging {
*/
def / (other: Any): Column = withExpr { Divide(expr, lit(other).expr) }

/**
* Integer Division this expression by another expression.
* {{{
* // Scala: The following divides a person's height by their weight.
* people.select( people("height") div people("weight") )
*
* // Java:
* people.select( people("height").div(people("weight")) );
* }}}
*
* @group expr_ops
* @since 2.1.0
*/
def div (other: Any): Column = withExpr { IntegerDivide(expr, lit(other).expr) }
Copy link
Contributor

Choose a reason for hiding this comment

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

why can't we just use the normal / ?


/**
* Division this expression by another expression.
* {{{
Expand Down