[SPARK-18053][SQL] compare unsafe and safe complex-type values correctly #15929
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Author
|
Test build #68843 has finished for PR 15929 at commit
|
hvanhovell
reviewed
Nov 18, 2016
| val funcCode: String = | ||
| s""" | ||
| public int $compareFunc(ArrayData a, ArrayData b) { | ||
| if (a instanceof UnsafeArrayData && b instanceof UnsafeArrayData && a == b) { |
Contributor
There was a problem hiding this comment.
Why the instanceof checks? This is java, you are comparing by reference.
gatorsmile
reviewed
Nov 19, 2016
| case dt: DataType if isPrimitiveType(dt) => s"$c1 == $c2" | ||
| case dt: DataType if dt.isInstanceOf[AtomicType] => s"$c1.equals($c2)" | ||
| case array: ArrayType => genComp(array, c1, c2) + " == 0" | ||
| case struct: StructType => genComp(struct, c1, c2) + " == 0" |
Contributor
There was a problem hiding this comment.
MapType is not comparable. We currently do not support equals() nor hashcode() for MapData. See https://issues.apache.org/jira/browse/SPARK-18134 for a fun discussion on this.
Member
There was a problem hiding this comment.
: ) That is a fun discussion.
Also accidentally found how Preso did it in a PR: https://github.com/prestodb/presto/pull/2469/files
Member
There was a problem hiding this comment.
If we do not plan to support MapType, could we add a negative test case?
gatorsmile
reviewed
Nov 19, 2016
| } else if (left.dataType != BinaryType) { | ||
| input1 == input2 | ||
| } else { | ||
| java.util.Arrays.equals(input1.asInstanceOf[Array[Byte]], input2.asInstanceOf[Array[Byte]]) |
Member
There was a problem hiding this comment.
This is a bug, right? I mean the previous code.
Contributor
Author
There was a problem hiding this comment.
yea, we may compare an unsafe and safe row/array/map previously.
|
Test build #68915 has finished for PR 15929 at commit
|
Contributor
|
LGTM. Merging to master/2.1/2.0. Thanks! |
asfgit
pushed a commit
that referenced
this pull request
Nov 23, 2016
## What changes were proposed in this pull request? In Spark SQL, some expression may output safe format values, e.g. `CreateArray`, `CreateStruct`, `Cast`, etc. When we compare 2 values, we should be able to compare safe and unsafe formats. The `GreaterThan`, `LessThan`, etc. in Spark SQL already handles it, but the `EqualTo` doesn't. This PR fixes it. ## How was this patch tested? new unit test and regression test Author: Wenchen Fan <[email protected]> Closes #15929 from cloud-fan/type-aware. (cherry picked from commit 84284e8) Signed-off-by: Herman van Hovell <[email protected]>
asfgit
pushed a commit
that referenced
this pull request
Nov 23, 2016
## What changes were proposed in this pull request? In Spark SQL, some expression may output safe format values, e.g. `CreateArray`, `CreateStruct`, `Cast`, etc. When we compare 2 values, we should be able to compare safe and unsafe formats. The `GreaterThan`, `LessThan`, etc. in Spark SQL already handles it, but the `EqualTo` doesn't. This PR fixes it. ## How was this patch tested? new unit test and regression test Author: Wenchen Fan <[email protected]> Closes #15929 from cloud-fan/type-aware. (cherry picked from commit 84284e8) Signed-off-by: Herman van Hovell <[email protected]>
robert3005
pushed a commit
to palantir/spark
that referenced
this pull request
Dec 2, 2016
## What changes were proposed in this pull request? In Spark SQL, some expression may output safe format values, e.g. `CreateArray`, `CreateStruct`, `Cast`, etc. When we compare 2 values, we should be able to compare safe and unsafe formats. The `GreaterThan`, `LessThan`, etc. in Spark SQL already handles it, but the `EqualTo` doesn't. This PR fixes it. ## How was this patch tested? new unit test and regression test Author: Wenchen Fan <[email protected]> Closes apache#15929 from cloud-fan/type-aware.
uzadude
pushed a commit
to uzadude/spark
that referenced
this pull request
Jan 27, 2017
## What changes were proposed in this pull request? In Spark SQL, some expression may output safe format values, e.g. `CreateArray`, `CreateStruct`, `Cast`, etc. When we compare 2 values, we should be able to compare safe and unsafe formats. The `GreaterThan`, `LessThan`, etc. in Spark SQL already handles it, but the `EqualTo` doesn't. This PR fixes it. ## How was this patch tested? new unit test and regression test Author: Wenchen Fan <[email protected]> Closes apache#15929 from cloud-fan/type-aware.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
In Spark SQL, some expression may output safe format values, e.g.
CreateArray,CreateStruct,Cast, etc. When we compare 2 values, we should be able to compare safe and unsafe formats.The
GreaterThan,LessThan, etc. in Spark SQL already handles it, but theEqualTodoesn't. This PR fixes it.How was this patch tested?
new unit test and regression test