-
Notifications
You must be signed in to change notification settings - Fork 29k
[SQL] Miscellaneous SQL/DF expression changes. #6754
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 4 commits
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 |
|---|---|---|
|
|
@@ -91,6 +91,7 @@ trait HiveTypeCoercion { | |
| StringToIntegralCasts :: | ||
| FunctionArgumentConversion :: | ||
| CaseWhenCoercion :: | ||
| IfCoercion :: | ||
| Division :: | ||
| PropagateTypes :: | ||
| ExpectedInputConversion :: | ||
|
|
@@ -652,6 +653,27 @@ trait HiveTypeCoercion { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Coerces the type of different branches of If statement to a common type. | ||
| */ | ||
| object IfCoercion extends Rule[LogicalPlan] { | ||
| def apply(plan: LogicalPlan): LogicalPlan = plan transformAllExpressions { | ||
| // Find tightest common type for If, if the true value and false value have different types. | ||
| case i @ If(pred, left, right) if left.dataType != right.dataType => | ||
| findTightestCommonTypeOfTwo(left.dataType, right.dataType).map { widestType => | ||
| val newLeft = if (left.dataType == widestType) left else Cast(left, widestType) | ||
| val newRight = if (right.dataType == widestType) right else Cast(right, widestType) | ||
| i.makeCopy(Array(pred, newLeft, newRight)) | ||
| }.getOrElse(i) // If there is no applicable conversion, leave expression unchanged. | ||
|
|
||
| // Convert If(null literal, _, _) into boolean type. | ||
| // In the optimizer, we should short-circuit this directly into false value. | ||
| case i @ If(pred, left, right) if pred.dataType == NullType => | ||
| println("fireing this rule") | ||
| i.makeCopy(Array(Literal.create(null, BooleanType), 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. Why this special case? Also, I'd just use
Contributor
Author
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. "if(null, true, false)" gets a nulltype.
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. what about case i @ If (pred, _, _) if pred.dataType == NullType =>
i.copy(predicate = Literal.create(null, BooleanType))
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. +1 |
||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Casts types according to the expected input types for Expressions that have the trait | ||
| * `ExpectsInputTypes`. | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,7 +110,20 @@ class DataFrameFunctionsSuite extends QueryTest { | |
| testData2.collect().toSeq.map(r => Row(~r.getInt(0)))) | ||
| } | ||
|
|
||
| test("length") { | ||
| test("if function") { | ||
|
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 put the |
||
| val df = Seq((1, 2)).toDF("a", "b") | ||
| checkAnswer( | ||
| df.selectExpr("if(a = 1, 'one', 'not_one')", "if(b = 1, 'one', 'not_one')"), | ||
| Row("one", "not_one")) | ||
| } | ||
|
|
||
| test("nvl function") { | ||
| checkAnswer( | ||
| ctx.sql("SELECT nvl(null, 'x'), nvl('y', 'x'), nvl(null, null)"), | ||
| Row("x", "y", null)) | ||
| } | ||
|
|
||
| test("string length function") { | ||
| checkAnswer( | ||
| nullStrings.select(strlen($"s"), strlen("s")), | ||
| nullStrings.collect().toSeq.map { r => | ||
|
|
@@ -127,18 +140,4 @@ class DataFrameFunctionsSuite extends QueryTest { | |
| Row(l) | ||
| }) | ||
| } | ||
|
|
||
| test("log2 functions test") { | ||
| val df = Seq((1, 2)).toDF("a", "b") | ||
| checkAnswer( | ||
| df.select(log2("b") + log2("a")), | ||
| Row(1)) | ||
|
|
||
| checkAnswer( | ||
| ctx.sql("SELECT LOG2(8)"), | ||
| Row(3)) | ||
| checkAnswer( | ||
| ctx.sql("SELECT LOG2(null)"), | ||
| Row(null)) | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ | |
| package org.apache.spark.sql | ||
|
|
||
| import org.apache.spark.sql.functions._ | ||
| import org.apache.spark.sql.functions.{log => logarithm} | ||
|
|
||
|
|
||
| private object MathExpressionsTestData { | ||
|
|
@@ -151,20 +152,31 @@ class MathExpressionsSuite extends QueryTest { | |
| testOneToOneMathFunction(tanh, math.tanh) | ||
| } | ||
|
|
||
| test("toDeg") { | ||
| test("toDegrees") { | ||
| testOneToOneMathFunction(toDegrees, math.toDegrees) | ||
| checkAnswer( | ||
| ctx.sql("SELECT degrees(0), degrees(1), degrees(1.5)"), | ||
| Seq((1, 2)).toDF().select(toDegrees(lit(0)), toDegrees(lit(1)), toDegrees(lit(1.5))) | ||
|
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. Should these tests not be using the same code path to generate the correct answer?
Contributor
Author
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. This was mainly testing function registry. The above one (testOneToOneMathFunction) tests correctness |
||
| ) | ||
| } | ||
|
|
||
| test("toRad") { | ||
| test("toRadians") { | ||
| testOneToOneMathFunction(toRadians, math.toRadians) | ||
| checkAnswer( | ||
| ctx.sql("SELECT radians(0), radians(1), radians(1.5)"), | ||
| Seq((1, 2)).toDF().select(toRadians(lit(0)), toRadians(lit(1)), toRadians(lit(1.5))) | ||
| ) | ||
| } | ||
|
|
||
| test("cbrt") { | ||
| testOneToOneMathFunction(cbrt, math.cbrt) | ||
| } | ||
|
|
||
| test("ceil") { | ||
| test("ceil and ceiling") { | ||
| testOneToOneMathFunction(ceil, math.ceil) | ||
| checkAnswer( | ||
| ctx.sql("SELECT ceiling(0), ceiling(1), ceiling(1.5)"), | ||
| Row(0.0, 1.0, 2.0)) | ||
| } | ||
|
|
||
| test("floor") { | ||
|
|
@@ -183,12 +195,21 @@ class MathExpressionsSuite extends QueryTest { | |
| testOneToOneMathFunction(expm1, math.expm1) | ||
| } | ||
|
|
||
| test("signum") { | ||
| test("signum / sign") { | ||
| testOneToOneMathFunction[Double](signum, math.signum) | ||
|
|
||
| checkAnswer( | ||
| ctx.sql("SELECT sign(10), signum(-11)"), | ||
| Row(1, -1)) | ||
| } | ||
|
|
||
| test("pow") { | ||
| test("pow / power") { | ||
| testTwoToOneMathFunction(pow, pow, math.pow) | ||
|
|
||
| checkAnswer( | ||
| ctx.sql("SELECT pow(1, 2), power(2, 1)"), | ||
| Seq((1, 2)).toDF().select(pow(lit(1), lit(2)), pow(lit(2), lit(1))) | ||
| ) | ||
| } | ||
|
|
||
| test("hypot") { | ||
|
|
@@ -199,8 +220,12 @@ class MathExpressionsSuite extends QueryTest { | |
| testTwoToOneMathFunction(atan2, atan2, math.atan2) | ||
| } | ||
|
|
||
| test("log") { | ||
| test("log / ln") { | ||
| testOneToOneNonNegativeMathFunction(org.apache.spark.sql.functions.log, math.log) | ||
| checkAnswer( | ||
| ctx.sql("SELECT ln(0), ln(1), ln(1.5)"), | ||
| Seq((1, 2)).toDF().select(logarithm(lit(0)), logarithm(lit(1)), logarithm(lit(1.5))) | ||
| ) | ||
| } | ||
|
|
||
| test("log10") { | ||
|
|
@@ -211,4 +236,18 @@ class MathExpressionsSuite extends QueryTest { | |
| testOneToOneNonNegativeMathFunction(log1p, math.log1p) | ||
| } | ||
|
|
||
| test("log2") { | ||
| val df = Seq((1, 2)).toDF("a", "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. I agree that we need to add tests here since they are math expressions, but I think we'd better keep tests in this suite consistent. These tests added here are in the style of DataFrameFunctionsSuite.
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 this change is a great idea, as it's more straightforward when reading the code, otherwise we probably need to jump back and forth.
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. Sort of...
Contributor
Author
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 it makes sense to put all math stuff in here, all string stuff into its own suite, etc basically if we group expressions into files; we should also group test suites in the same way.
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 makes sense. |
||
| checkAnswer( | ||
| df.select(log2("b") + log2("a")), | ||
| Row(1)) | ||
|
|
||
| checkAnswer(ctx.sql("SELECT LOG2(8), LOG2(null)"), Row(3, null)) | ||
| } | ||
|
|
||
| test("negative") { | ||
| checkAnswer( | ||
| ctx.sql("SELECT negative(1), negative(0), negative(-1)"), | ||
| Row(-1, 0, 1)) | ||
| } | ||
| } | ||
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.
Hive support promote types to String for
If, but ourfindTightestCommonTypeOfTwodoesn't support it. There are some other expressions likeCoalesce,CaseWhenneed to promote types to String. #6551 tries to fix it but it's not complete. I'm wondering if hive has a specific rule about string type promotion that we can follow, or is it all about hive's implicit conversions?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.
i think we can define new TypeConversion class for Coalesce, If, CaseWhen, and so on. because in Hive these udf function use same one type Conversion rule as https://github.com/apache/hive/blob/ac755ebe26361a4647d53db2a28500f71697b276/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFUtils.java#L79.
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.
yup that's a good idea.
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.
I added a rule to promote to string just for If for this patch. We can generalize it later.