Skip to content

Conversation

@srowen
Copy link
Member

@srowen srowen commented Sep 10, 2018

What changes were proposed in this pull request?

Correct some comparisons between unrelated types to what they seem to… have been trying to do

How was this patch tested?

Existing tests.

// other places, so drop them to reduce the memory usage.
!acc.internal && (!acc.metadata.isDefined ||
acc.metadata.get != Some(AccumulatorContext.SQL_ACCUM_IDENTIFIER))
!acc.internal && acc.metadata != Some(AccumulatorContext.SQL_ACCUM_IDENTIFIER)
Copy link
Member Author

Choose a reason for hiding this comment

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

CC @vanzin on this one for a double-check

closure
.getClass
.getInterfaces.exists(_.getName.equals("scala.Serializable"))
.getInterfaces.exists(_.getName == "scala.Serializable")
Copy link
Member Author

Choose a reason for hiding this comment

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

See JIRA for explanation of this one

.select('a)
.where('a > 1)
.where('a != 200)
.where('a =!= 200)
Copy link
Member Author

Choose a reason for hiding this comment

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

(!= isn't actually the Column operator for historical reasons having to do with precedence)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, thank you for fixing this.

assert(unsafeDate.numElements == dateArray.length)
dateArray.zipWithIndex.map { case (e, i) =>
assert(unsafeDate.get(i, DateType) == e)
assert(unsafeDate.get(i, DateType).asInstanceOf[Int] == e)
Copy link
Member Author

Choose a reason for hiding this comment

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

Issue here is that you can't really compare AnyRef to a primitive, not via boxing or unboxing; this may just be making some Scala coercion explicit but seemed worthwhile

@SparkQA
Copy link

SparkQA commented Sep 11, 2018

Test build #95897 has finished for PR 22384 at commit 9e70b62.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Sep 11, 2018

Thanks, @srowen . I also checked that this PR resolves all existing Comparing unrelated types issues via IntellJ, what about removing all tags like [CORE][MESOS] since it touched more modules, sql and resouce-manager/yarn?

@SparkQA
Copy link

SparkQA commented Sep 11, 2018

Test build #4334 has finished for PR 22384 at commit 9e70b62.

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

@srowen srowen changed the title [SPARK-25398][CORE][MESOS] Minor bugs from comparing unrelated types [SPARK-25398] Minor bugs from comparing unrelated types Sep 11, 2018
@vanzin
Copy link
Contributor

vanzin commented Sep 11, 2018

Looks fine.

@srowen
Copy link
Member Author

srowen commented Sep 11, 2018

Merged to master/2.4

@asfgit asfgit closed this in cfbdd6a Sep 11, 2018
asfgit pushed a commit that referenced this pull request Sep 11, 2018
## What changes were proposed in this pull request?

Correct some comparisons between unrelated types to what they seem to… have been trying to do

## How was this patch tested?

Existing tests.

Closes #22384 from srowen/SPARK-25398.

Authored-by: Sean Owen <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
(cherry picked from commit cfbdd6a)
Signed-off-by: Sean Owen <[email protected]>
fjh100456 pushed a commit to fjh100456/spark that referenced this pull request Sep 13, 2018
## What changes were proposed in this pull request?

Correct some comparisons between unrelated types to what they seem to… have been trying to do

## How was this patch tested?

Existing tests.

Closes apache#22384 from srowen/SPARK-25398.

Authored-by: Sean Owen <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
@srowen srowen deleted the SPARK-25398 branch September 20, 2018 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants