Skip to content

Conversation

@10110346
Copy link
Contributor

What changes were proposed in this pull request?

This PR adds a unit test for OpenHashMap , this can help developers to distinguish between the 0/0.0/0L and null

How was this patch tested?

Copy link
Member

Choose a reason for hiding this comment

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

What do these tests mean? Tests for different type comparisons? e.g.,

    assert(!Some(specializedMap1("a")).contains(0.0))
    assert(!Some(specializedMap1("b")).contains(0.toShort))

Copy link
Contributor Author

@10110346 10110346 Aug 27, 2018

Choose a reason for hiding this comment

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

if the data type is in @specialized annotation , and the key is not be contained ,the map(key) will return 0
if the data type is not in @specialized annotation , and the key is not be contained ,the map(key) will return null

Copy link
Member

Choose a reason for hiding this comment

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

ah, I see. So, can you describe more in the description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok , i will add it

@maropu
Copy link
Member

maropu commented Aug 27, 2018

plz add [CORE] in the title?

@10110346 10110346 changed the title [SPARK-25249][TEST]add a unit test for OpenHashMap [SPARK-25249][CORE][TEST]add a unit test for OpenHashMap Aug 27, 2018
@10110346
Copy link
Contributor Author

thanks @maropu

@SparkQA
Copy link

SparkQA commented Aug 27, 2018

Test build #95282 has finished for PR 22241 at commit 76b9f48.

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

@SparkQA
Copy link

SparkQA commented Aug 27, 2018

Test build #95285 has finished for PR 22241 at commit 8aea12e.

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

@kiszk
Copy link
Member

kiszk commented Aug 27, 2018

Adding tests looks good.
Probably beyond the purpose of this PR, is the following difference intentionally designed?

    // If the data type is in @specialized annotation, and
    // the `key` is not be contained, the `map(key)` will return 0
    // If the data type is not in @specialized annotation, and
    // the `key` is not be contained, the `map(key)` will return null

@srowen
Copy link
Member

srowen commented Aug 27, 2018

@kiszk I guess it's because in this case the underlying value type is a primitive like int or long, so null can't be returned?

@srowen
Copy link
Member

srowen commented Aug 27, 2018

Merged to master

@asfgit asfgit closed this in 381a967 Aug 27, 2018
@SparkQA
Copy link

SparkQA commented Aug 27, 2018

Test build #95286 has finished for PR 22241 at commit 9346112.

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

bogdanrdc pushed a commit to bogdanrdc/spark that referenced this pull request Aug 28, 2018
## What changes were proposed in this pull request?

This PR adds a unit test for OpenHashMap , this can help developers  to distinguish between the 0/0.0/0L and null

## How was this patch tested?

Closes apache#22241 from 10110346/openhashmap.

Authored-by: liuxian <[email protected]>
Signed-off-by: Sean Owen <[email protected]>
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.

6 participants