Skip to content

[SPARK-16729][SQL] Throw analysis exception for invalid date casts - #14358

Closed
petermaxlee wants to merge 2 commits into
apache:masterfrom
petermaxlee:SPARK-16729
Closed

[SPARK-16729][SQL] Throw analysis exception for invalid date casts#14358
petermaxlee wants to merge 2 commits into
apache:masterfrom
petermaxlee:SPARK-16729

Conversation

@petermaxlee

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Spark currently throws exceptions for invalid casts for all other data types except date type. Somehow date type returns null. It should be consistent and throws analysis exception as well.

How was this patch tested?

Added a unit test case in CastSuite.

@SparkQA

Copy link
Copy Markdown

Test build #62857 has finished for PR 14358 at commit 5419b85.

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

@petermaxlee

Copy link
Copy Markdown
ContributorAuthor

@cloud-fan can you take a look?

// It is never possible to compare result when hive return with exception,
// so we can return null
// NULL is more reasonable here, since the query itself obeys the grammar.
case _ => _ => null

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cc @yhuai@liancheng , do you remember why we have this behaviour at the beginning?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like a design decision made in the original PR. See here: https://github.com/apache/spark/pull/2344/files#diff-258b71121d8d168e4d53cb5b6dc53ffeR166

I don't think we've ever discussed this case explicitly. This change seems reasonable to me.

@petermaxlee

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

// to ensure we test every possible cast situation here
atomicTypes.zip(atomicTypes).foreach { case (from, to) =>
checkNullCast(from, to)
if (Cast.canCast(from, to)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why this check? doesn;t from always equal to to here?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Not all atomicTypes can cast from each other? E.g. date.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

def canCast(from: DataType, to: DataType): Boolean = (from, to) match {
case (fromType, toType) if fromType == toType => true
......

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

ah this is doing self casting - i read it wrong. let me remove it.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

removed

@cloud-fan

Copy link
Copy Markdown
Contributor

LGTM, pending jenkins.

@SparkQA

Copy link
Copy Markdown

Test build #62910 has finished for PR 14358 at commit 0161896.

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

@cloud-fan

Copy link
Copy Markdown
Contributor

thanks, merging to master!

@rxin

rxin commented Jul 27, 2016

Copy link
Copy Markdown
Contributor

I would consider this a bug and put it in branch-2.0. I'm going to cherry pick.

asfgit pushed a commit that referenced this pull request Jul 27, 2016
Spark currently throws exceptions for invalid casts for all other data types except date type. Somehow date type returns null. It should be consistent and throws analysis exception as well.
Added a unit test case in CastSuite.
Author: petermaxlee <petermaxlee@gmail.com>
Closes#14358 from petermaxlee/SPARK-16729.
(cherry picked from commit ef0ccbc)
Signed-off-by: Reynold Xin <rxin@databricks.com>
Sign up for freeto 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

@petermaxlee@SparkQA@cloud-fan@rxin@liancheng