Skip to content

Conversation

@maropu
Copy link
Member

@maropu maropu commented Jan 12, 2018

What changes were proposed in this pull request?

This pr fixed code to compare values in compareAndGetNewStats.
The test below fails in the current master;

    val oldStats2 = CatalogStatistics(sizeInBytes = BigInt(Long.MaxValue) * 2)
    val newStats5 = CommandUtils.compareAndGetNewStats(
      Some(oldStats2), newTotalSize = BigInt(Long.MaxValue) * 2, None)
    assert(newStats5.isEmpty)

How was this patch tested?

Added some tests in CommandUtilsSuite.

@maropu
Copy link
Member Author

maropu commented Jan 12, 2018

nitpicking though, could you check? @gatorsmile @wzhfy @mbasmanova

@SparkQA
Copy link

SparkQA commented Jan 12, 2018

Test build #86019 has finished for PR 20245 at commit 473b37a.

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

@maropu
Copy link
Member Author

maropu commented Jan 12, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Jan 12, 2018

Test build #86024 has finished for PR 20245 at commit 473b37a.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds no public classes.

@maropu
Copy link
Member Author

maropu commented Jan 12, 2018

retest this please

@SparkQA
Copy link

SparkQA commented Jan 12, 2018

Test build #86029 has finished for PR 20245 at commit 473b37a.

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

@srowen
Copy link
Member

srowen commented Jan 12, 2018

Do these values actually take on values larger than Long.MaxValue?

@maropu
Copy link
Member Author

maropu commented Jan 12, 2018

yea, it's a super corner-case (almost impossible). So, this is just a correctness issue as intellij suggests incompatible comparisons. (Actually, I'm not sure why we use BigInt for the statistics for Long)

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

Although this PR is to fix a corner case, we still should clean it.

Thanks! Merged to master/2.3

asfgit pushed a commit that referenced this pull request Jan 13, 2018
…compareAndGetNewStats

## What changes were proposed in this pull request?
This pr fixed code to compare values in `compareAndGetNewStats`.
The test below fails in the current master;
```
    val oldStats2 = CatalogStatistics(sizeInBytes = BigInt(Long.MaxValue) * 2)
    val newStats5 = CommandUtils.compareAndGetNewStats(
      Some(oldStats2), newTotalSize = BigInt(Long.MaxValue) * 2, None)
    assert(newStats5.isEmpty)
```

## How was this patch tested?
Added some tests in `CommandUtilsSuite`.

Author: Takeshi Yamamuro <[email protected]>

Closes #20245 from maropu/SPARK-21213-FOLLOWUP.

(cherry picked from commit 0066d6f)
Signed-off-by: gatorsmile <[email protected]>
@asfgit asfgit closed this in 0066d6f Jan 13, 2018
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