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-9549][SQL] fix bugs in expressions #7882

Closed
wants to merge 3 commits into from

Conversation

yjshen
Copy link
Member

@yjshen yjshen commented Aug 3, 2015

JIRA: https://issues.apache.org/jira/browse/SPARK-9549

This PR fix the following bugs:

  1. UnaryMinus's codegen version would fail to compile when the input is Long.MinValue
  2. BinaryComparison would fail to compile in codegen mode when comparing Boolean types.
  3. AddMonth would fail if passed a huge negative month, which would lead accessing negative index of monthDays array.
  4. Nanvl with different type operands.

@@ -56,6 +56,7 @@ class ArithmeticExpressionSuite extends SparkFunSuite with ExpressionEvalHelper
checkEvaluation(UnaryMinus(input), convert(-1))
checkEvaluation(UnaryMinus(Literal.create(null, dataType)), null)
}
checkEvaluation(UnaryMinus(Literal.create(Long.MinValue, LongType)), Long.MinValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

add one for int/short/byte type too

case n @ NaNvl(l, r) if l.dataType != r.dataType =>
l.dataType match {
case DoubleType => NaNvl(l, Cast(r, DoubleType))
case FloatType => NaNvl(Cast(l, DoubleType), r)
Copy link
Contributor

Choose a reason for hiding this comment

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

could this throw a bad exception if the input is some other type that is not Double/Float?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have this in Nanvl:

override def inputTypes: Seq[AbstractDataType] =
    Seq(TypeCollection(DoubleType, FloatType), TypeCollection(DoubleType, FloatType))

Copy link
Contributor

Choose a reason for hiding this comment

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

can you double check that FunctionArgumentConversion is not called before the implicit cast rule?

@SparkQA
Copy link

SparkQA commented Aug 3, 2015

Test build #39500 has finished for PR 7882 at commit 4fa5de0.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Aug 3, 2015

Test build #39505 has finished for PR 7882 at commit 3dee204.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Aug 3, 2015

LGTM

@SparkQA
Copy link

SparkQA commented Aug 3, 2015

Test build #39514 has finished for PR 7882 at commit 41bbd2c.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@rxin
Copy link
Contributor

rxin commented Aug 3, 2015

Thanks - I've merged this.

val currentYear = absoluteMonth / 12
val nonNegativeMonth = if (absoluteMonth >= 0) absoluteMonth else 0
val currentMonthInYear = nonNegativeMonth % 12
val currentYear = nonNegativeMonth / 12
Copy link
Contributor

Choose a reason for hiding this comment

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

The above two statements can be replaced with:
val (currentYear, currentMonthInYear) = nonNegativeMonth /% 12

Copy link
Contributor

Choose a reason for hiding this comment

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

How is that possible?

Copy link
Contributor

Choose a reason for hiding this comment

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

/% is not defined for Int.

I read the notion in a Scala book which I have returned. I will read more once I have that book back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants