Skip to content

Comments

[SPARK-21205][SQL] pmod(number, 0) should be null.#18413

Closed
wangyum wants to merge 4 commits intoapache:masterfrom
wangyum:SPARK-21205
Closed

[SPARK-21205][SQL] pmod(number, 0) should be null.#18413
wangyum wants to merge 4 commits intoapache:masterfrom
wangyum:SPARK-21205

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Jun 25, 2017

What changes were proposed in this pull request?

Hive pmod(3.13, 0):

hive> select pmod(3.13, 0);
OK
NULL
Time taken: 2.514 seconds, Fetched: 1 row(s)
hive> 

Spark mod(3.13, 0):

spark-sql> select mod(3.13, 0);
NULL
spark-sql> 

But the Spark pmod(3.13, 0):

spark-sql> select pmod(3.13, 0);
17/06/25 09:35:58 ERROR SparkSQLDriver: Failed in [select pmod(3.13, 0)]
java.lang.NullPointerException
	at org.apache.spark.sql.catalyst.expressions.Pmod.pmod(arithmetic.scala:504)
	at org.apache.spark.sql.catalyst.expressions.Pmod.nullSafeEval(arithmetic.scala:432)
	at org.apache.spark.sql.catalyst.expressions.BinaryExpression.eval(Expression.scala:419)
	at org.apache.spark.sql.catalyst.expressions.UnaryExpression.eval(Expression.scala:323)
...

This PR make pmod(number, 0) to null.

How was this patch tested?

unit tests

@SparkQA
Copy link

SparkQA commented Jun 25, 2017

Test build #78572 has finished for PR 18413 at commit ffd3bcb.

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

@srowen
Copy link
Member

srowen commented Jun 25, 2017

I don't know enough to review the change but I believe you're correct. Dividing by zero also results in NULL right?

@SparkQA
Copy link

SparkQA commented Jun 25, 2017

Test build #78584 has finished for PR 18413 at commit 9bad90f.

  • This patch fails to build.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 26, 2017

Test build #78587 has finished for PR 18413 at commit 508c3aa.

  • This patch fails PySpark pip packaging tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@gatorsmile
Copy link
Member

cc @wzhfy

@wangyum
Copy link
Member Author

wangyum commented Jun 28, 2017

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Jun 28, 2017

Test build #78785 has finished for PR 18413 at commit 508c3aa.

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

@wangyum
Copy link
Member Author

wangyum commented Jun 29, 2017

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Jun 29, 2017

Test build #78911 has finished for PR 18413 at commit 508c3aa.

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

@SparkQA
Copy link

SparkQA commented Jul 3, 2017

Test build #79073 has finished for PR 18413 at commit da037c8.

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

@wangyum
Copy link
Member Author

wangyum commented Jul 3, 2017

Jenkins, retest this please

@SparkQA
Copy link

SparkQA commented Jul 3, 2017

Test build #79087 has finished for PR 18413 at commit da037c8.

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

@wangyum
Copy link
Member Author

wangyum commented Aug 3, 2017

@HyukjinKwon Could you help review this?

@hvanhovell
Copy link
Contributor

retest this please

@SparkQA
Copy link

SparkQA commented Aug 4, 2017

Test build #80240 has finished for PR 18413 at commit da037c8.

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

Copy link
Contributor

@hvanhovell hvanhovell left a comment

Choose a reason for hiding this comment

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

LGTM - merging to master

@asfgit asfgit closed this in 231f672 Aug 4, 2017
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