Skip to content

[SPARK-14541] [SQL] SQL function: IFNULL, NULLIF, NVL and NVL2#12373

Closed
bomeng wants to merge 9 commits intoapache:masterfrom
bomeng:SPARK-14541
Closed

[SPARK-14541] [SQL] SQL function: IFNULL, NULLIF, NVL and NVL2#12373
bomeng wants to merge 9 commits intoapache:masterfrom
bomeng:SPARK-14541

Conversation

@bomeng
Copy link
Contributor

@bomeng bomeng commented Apr 13, 2016 •
edited
Loading

What changes were proposed in this pull request?

I am trying to implement functions NULLIF in this PR. The meaning of NULLIF can be found here:
NULLIF( )
NVL( ) / IFNULL( ) IFNULL() and NVL() are the same.
NVL2( )

How was this patch tested?

Test cases were added.

@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55758 has finished for PR 12373 at commit c479394.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class NullIf(left: Expression, right: Expression) extends BinaryExpression

@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55773 has finished for PR 12373 at commit fc144ee.

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

viirya
viirya reviewed Apr 14, 2016
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/nullExpressions.scala

override def genCode(ctx: CodegenContext, ev: ExprCode): String = {
val leftGen = left.gen(ctx)
val rightGen = right.gen(ctx)
Copy link
Member

@viirya viirya Apr 14, 2016

Choose a reason for hiding this comment

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

We have CodegenContext.genEqual to generate codes for equality check. You can see how EqualTo expression uses it.

Copy link
Contributor Author

@bomeng bomeng Apr 14, 2016

Choose a reason for hiding this comment

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

Thanks, @viirya ! That simplifies the logic!

@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55786 has finished for PR 12373 at commit aedcbf3.

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

@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55832 has finished for PR 12373 at commit 93bff09.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Nvl(left: Expression, right: Expression) extends BinaryExpression

@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55845 has finished for PR 12373 at commit de65cf4.

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

@bomeng bomeng changed the title [SPARK-14541] [SQL] [WIP] SQL function: IFNULL, NULLIF, NVL and NVL2 [SPARK-14541] [SQL] SQL function: IFNULL, NULLIF, NVL and NVL2 Apr 14, 2016
@SparkQA
Copy link

SparkQA commented Apr 14, 2016

Test build #55850 has finished for PR 12373 at commit 04a361a.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Nvl2(first: Expression, second: Expression, third: Expression)

davies
davies reviewed Apr 15, 2016
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/functions.scala
* @group normal_funcs
* @since 2.0.0
*/
def nvl(col1: Column, col2: Column): Column = withExpr { Nvl(col1.expr, col2.expr)}
Copy link
Contributor

@davies davies Apr 15, 2016

Choose a reason for hiding this comment

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

docs

@rxin
Copy link
Contributor

rxin commented Apr 15, 2016

Also can you remove the "JIRA related" section from the description? It is redundant with the title when makes into the commit message.

@bomeng
Copy link
Contributor Author

bomeng commented Apr 15, 2016

I will address these issues tomorrow! Thank you all!

@bomeng
Copy link
Contributor Author

bomeng commented Apr 15, 2016 •
edited
Loading

I have revisited the codes and made the codes more robust. Heavily tested against different data types by introducing testAllTypes2Values() with 2 different values. PR description was updated. Javadoc was fixed. Please leave comments!

@SparkQA
Copy link

SparkQA commented Apr 15, 2016

Test build #55947 has finished for PR 12373 at commit 451e5fb.

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

@SparkQA
Copy link

SparkQA commented Apr 15, 2016

Test build #55948 has finished for PR 12373 at commit 4e21e1a.

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

@bomeng
Copy link
Contributor Author

bomeng commented Apr 19, 2016 •
edited
Loading

@davies @rxin Could you please take a look if you get a chance? Thanks.

@rxin rxin mentioned this pull request May 12, 2016
Closed
@rxin
Copy link
Contributor

rxin commented May 12, 2016

I added #13084

I thought about it more -- since it is unclear how often people will actually use these functions, it's probably not worth following strictly the corner case behaviors in Oracle/MSSQL. I created a new small framework that allows replacing the expressions at runtime with existing expressions, so we don't need to re-implement codegen.

asfgit pushed a commit that referenced this pull request May 13, 2016
@rxin @yhuai
[SPARK-14541][SQL] Support IFNULL, NULLIF, NVL and NVL2
51706f8 
## What changes were proposed in this pull request?
This patch adds support for a few SQL functions to improve compatibility with other databases: IFNULL, NULLIF, NVL and NVL2. In order to do this, this patch introduced a RuntimeReplaceable expression trait that allows replacing an unevaluable expression in the optimizer before evaluation.

Note that the semantics are not completely identical to other databases in esoteric cases.

## How was this patch tested?
Added a new test suite SQLCompatibilityFunctionSuite.

Closes #12373.

Author: Reynold Xin <rxin@databricks.com>

Closes #13084 from rxin/SPARK-14541.

(cherry picked from commit eda2800)
Signed-off-by: Yin Huai <yhuai@databricks.com>
@asfgit asfgit closed this in eda2800 May 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@bomeng @SparkQA @rxin @davies @viirya