-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-7952][SPARK-7984][SQL] equality check between boolean type and numeric type is broken. #6505
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
2846a04
fc0d741
9ba2130
625973c
ebc8c61
b6401ba
77f0f39
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 |
|---|---|---|
|
|
@@ -76,7 +76,7 @@ trait HiveTypeCoercion { | |
| WidenTypes :: | ||
| PromoteStrings :: | ||
| DecimalPrecision :: | ||
| BooleanComparisons :: | ||
| BooleanEqualization :: | ||
| StringToIntegralCasts :: | ||
| FunctionArgumentConversion :: | ||
| CaseWhenCoercion :: | ||
|
|
@@ -482,30 +482,49 @@ trait HiveTypeCoercion { | |
| } | ||
|
|
||
| /** | ||
| * Changes Boolean values to Bytes so that expressions like true < false can be Evaluated. | ||
| * Changes numeric values to booleans so that expressions like true = 1 can be evaluated. | ||
| */ | ||
| object BooleanComparisons extends Rule[LogicalPlan] { | ||
| val trueValues = Seq(1, 1L, 1.toByte, 1.toShort, new java.math.BigDecimal(1)).map(Literal(_)) | ||
| val falseValues = Seq(0, 0L, 0.toByte, 0.toShort, new java.math.BigDecimal(0)).map(Literal(_)) | ||
| object BooleanEqualization extends Rule[LogicalPlan] { | ||
| val trueValue = Literal(new java.math.BigDecimal(1)) | ||
| val falseValue = Literal(new java.math.BigDecimal(0)) | ||
|
|
||
| private def buildCaseKeyWhen(booleanExpr: Expression, numericExpr: Expression) = { | ||
| CaseKeyWhen(Cast(numericExpr, DecimalType.Unlimited), | ||
| Seq( | ||
| trueValue, booleanExpr, | ||
| falseValue, Not(booleanExpr), | ||
| Literal(false))) | ||
| } | ||
|
|
||
| private def transform(booleanExpr: Expression, numericExpr: Expression) = { | ||
| CaseWhen(Seq( | ||
| Or(IsNull(booleanExpr), IsNull(numericExpr)), Literal.create(null, BooleanType), | ||
| buildCaseKeyWhen(booleanExpr, numericExpr) | ||
| )) | ||
|
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. Seems that we can replace this |
||
| } | ||
|
|
||
| private def transformNullSafe(booleanExpr: Expression, numericExpr: Expression) = { | ||
| CaseWhen(Seq( | ||
| And(IsNull(booleanExpr), IsNull(numericExpr)), Literal(true), | ||
|
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. boolean is already comparable, we don't need to cast it to byte. |
||
| Or(IsNull(booleanExpr), IsNull(numericExpr)), Literal(false), | ||
| buildCaseKeyWhen(booleanExpr, numericExpr) | ||
| )) | ||
| } | ||
|
|
||
| def apply(plan: LogicalPlan): LogicalPlan = plan transformAllExpressions { | ||
| // Skip nodes who's children have not been resolved yet. | ||
| case e if !e.childrenResolved => e | ||
|
|
||
| // Hive treats (true = 1) as true and (false = 0) as true. | ||
| case EqualTo(l @ BooleanType(), r) if trueValues.contains(r) => l | ||
| case EqualTo(l, r @ BooleanType()) if trueValues.contains(l) => r | ||
| case EqualTo(l @ BooleanType(), r) if falseValues.contains(r) => Not(l) | ||
| case EqualTo(l, r @ BooleanType()) if falseValues.contains(l) => Not(r) | ||
|
|
||
| // No need to change other EqualTo operators as that actually makes sense for boolean types. | ||
| case e: EqualTo => e | ||
| // No need to change the EqualNullSafe operators, too | ||
| case e: EqualNullSafe => e | ||
| // Otherwise turn them to Byte types so that there exists and ordering. | ||
| case p: BinaryComparison if p.left.dataType == BooleanType && | ||
| p.right.dataType == BooleanType => | ||
| p.makeCopy(Array(Cast(p.left, ByteType), Cast(p.right, ByteType))) | ||
| // Hive treats (true = 1) as true and (false = 0) as true, | ||
| // all other cases are considered as false. | ||
| case EqualTo(l @ BooleanType(), r) if r.dataType.isInstanceOf[NumericType] => | ||
|
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 case case EqualTo(l @ BooleanType(), r @ NumericType) =>?
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. Also I think you are technically changing the semantics of this rule. It was a simplification rule before. In the case of this gets simplified to just But now it gets turned into a more complicated expression Can you describe clearly what bug you are trying to fix?
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. The origin logic is not right. How can we know the value of
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. We know the value of b if b is a literal value. Why is it wrong?
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. Hmm... Ok, we should add these cases back as a optimization for literal values. |
||
| transform(l, r) | ||
| case EqualTo(l, r @ BooleanType()) if l.dataType.isInstanceOf[NumericType] => | ||
| transform(r, l) | ||
| case EqualNullSafe(l @ BooleanType(), r) if r.dataType.isInstanceOf[NumericType] => | ||
| transformNullSafe(l, r) | ||
| case EqualNullSafe(l, r @ BooleanType()) if l.dataType.isInstanceOf[NumericType] => | ||
| transformNullSafe(r, l) | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1331,4 +1331,25 @@ class SQLQuerySuite extends QueryTest with BeforeAndAfterAll { | |
|
|
||
| checkAnswer(sql("SELECT a.`c.b`, `b.$q`[0].`a@!.q`, `q.w`.`w.i&`[0] FROM t"), Row(1, 1, 1)) | ||
| } | ||
|
|
||
| test("SPARK-7952: fix the equality check between boolean and numeric types") { | ||
| val df = Seq( | ||
| (1, true), | ||
| (0, false), | ||
| (2, true), | ||
| (2, false), | ||
| (null, true), | ||
| (null, false), | ||
| (0, null), | ||
| (1, null), | ||
| (null, null) | ||
| ).map { case (i, b) => | ||
| (i.asInstanceOf[Integer], b.asInstanceOf[java.lang.Boolean]) | ||
| }.toDF("i", "b") | ||
|
|
||
| checkAnswer(df.select('i === 'b), | ||
| Seq(true, true, false, false, null, null, null, null, null).map(Row(_))) | ||
| checkAnswer(df.select('i <=> 'b), | ||
| Seq(true, true, false, false, false, false, false, false, true).map(Row(_))) | ||
| } | ||
|
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. This test case can be simplified a bit, also please drop the temp table at last: val data = Seq(
(1, true),
(0, false),
(2, true),
(2, false),
(null, true),
(null, false),
(0, null),
(1, null),
(null, null)
).map { case (i, b) =>
(i.asInstanceOf[Integer], b.asInstanceOf[java.lang.Boolean])
}
data.toDF("i", "b").registerTempTable("t")
try {
// checkAnswer calls
} finally {
sqlCtx.dropTempTable("t")
} |
||
| } | ||
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.
Maybe we can do tighter bounds rather than going directly to Decimal? i.e. I'd consider casting to 1.toByte for byte values, and 1.toShort for short values, ...
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.
Actually we can do it after adding a rule to coercion types for key and when expressions in
CaseKeyWhen. Should I open a new PR to add that and improve this later?