Skip to content

Conversation

@ueshin
Copy link
Member

@ueshin ueshin commented Dec 3, 2014

This is a follow-up of SPARK-4593 (#3443).

@SparkQA
Copy link

SparkQA commented Dec 3, 2014

Test build #24089 has started for PR 3581 at commit c3959d4.

  • This patch merges cleanly.

@SparkQA
Copy link

SparkQA commented Dec 3, 2014

Test build #24089 has finished for PR 3581 at commit c3959d4.

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

@AmplabJenkins
Copy link

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/24089/
Test PASSed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remainder actually supports the fractional type, the previous implementation only support integral type, which is a bug, can you fix that also?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, forget about it, I just noticed that the i1 actually support the fractional type, too.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, may the support the fractional type in i1 is not so correct as its name suggested, probably it's better to add the data type checking here, as we did in Divide

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, rem seems only support the integral type, maybe that's why i1 accepts the NumericType instead.

@chenghao-intel
Copy link
Contributor

LGTM +1

@marmbrus
Copy link
Contributor

Thanks! Merged to master.

@asfgit asfgit closed this in ddc7ba3 Dec 17, 2014
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.

5 participants