Skip to content

[SPARK-16730][SQL] Implement function aliases for type casts - #14364

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

[SPARK-16730][SQL] Implement function aliases for type casts#14364
petermaxlee wants to merge 4 commits into
apache:masterfrom
petermaxlee:SPARK-16730-2

Conversation

@petermaxlee

@petermaxleepetermaxlee commented Jul 26, 2016

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Spark 1.x supports using the Hive type name as function names for doing casts, e.g.

SELECTint(1.0);
SELECT string(2.0);

The above query would work in Spark 1.x because Spark 1.x fail back to Hive for unimplemented functions, and break in Spark 2.0 because the fall back was removed.

This patch implements function aliases using an analyzer rule for the following cast functions:

  • boolean
  • tinyint
  • smallint
  • int
  • bigint
  • float
  • double
  • decimal
  • date
  • timestamp
  • binary
  • string

How was this patch tested?

Added end-to-end tests in SQLCompatibilityFunctionSuite.

@petermaxlee

petermaxlee commented Jul 26, 2016

Copy link
Copy Markdown
ContributorAuthor

@cloud-fan this is an alternative implementation using FunctionRegistry.

Let me know if you prefer this one over #14362.

cloud-fan
cloud-fan reviewed Jul 26, 2016
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
expression[BitwiseXor]("^"),

// Cast aliases (SPARK-16730)
castAlias("boolean", BooleanType),

@cloud-fancloud-fanJul 26, 2016

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.

in hive, if users create a udf called boolean, will hive throw exception or override the type casting one?

@petermaxleepetermaxleeJul 26, 2016

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.

boolean is just a normal function in Hive (same as for example acos), so it would do whatever a normal function's behavior is.

@SparkQA

SparkQA commented Jul 26, 2016

Copy link
Copy Markdown

Test build #62872 has finished for PR 14364 at commit 8b087b2.

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

@petermaxleepetermaxlee mentioned this pull request Jul 26, 2016
Closed
@SparkQA

SparkQA commented Jul 27, 2016

Copy link
Copy Markdown

Test build #62902 has finished for PR 14364 at commit b8fbcab.

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

cloud-fan
cloud-fan reviewed Jul 27, 2016
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
castAlias("tinyint", ByteType),
castAlias("smallint", ShortType),
castAlias("int", IntegerType),
castAlias("bigint", LongType),

@cloud-fancloud-fanJul 27, 2016

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.

use LongType.simpleString instead of bigint looks better. Same to others.

@petermaxleepetermaxleeJul 27, 2016

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.

I think that's actually worse, because it makes it less clear what the function name is by looking at this source file. Also if for some reason we change LongType.simpleString in the future, these functions will subtly break.

@cloud-fancloud-fanJul 27, 2016

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.

ok agree

@cloud-fan

cloud-fan commented Jul 27, 2016

Copy link
Copy Markdown
Contributor

mostly LGTM, thanks for working on it!

@SparkQA

SparkQA commented Jul 27, 2016

Copy link
Copy Markdown

Test build #62909 has finished for PR 14364 at commit 3b78da3.

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

hvanhovell
hvanhovell reviewed Jul 27, 2016
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala Outdated
}

/**
* Creates a function lookup registry for cast aliases (SPARK-16730).

@hvanhovellhvanhovellJul 27, 2016

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.

NIT: ...function lookup registry... should that ben...function registry lookup entry ... or something similar?

@hvanhovell

hvanhovell commented Jul 27, 2016

Copy link
Copy Markdown
Contributor

A few minor comments. LGTM otherwise.

@petermaxlee

petermaxlee commented Jul 27, 2016

Copy link
Copy Markdown
ContributorAuthor

I've updated the pull request based on @hvanhovell's comment.

@SparkQA

SparkQA commented Jul 27, 2016

Copy link
Copy Markdown

Test build #62928 has finished for PR 14364 at commit 5bbe995.

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

cloud-fan
cloud-fan reviewed Jul 28, 2016
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/FunctionRegistry.scala
}
Cast(args.head, dataType)
}
(name, (expressionInfo[Cast](name), builder))

@cloud-fancloud-fanJul 28, 2016

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.

so whatever cast function we describe, we will always show Cast's description right? Is it same with hive?

@petermaxleepetermaxleeJul 28, 2016

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.

Yes - this is a limitation. That's not what Hive does because Hive actually does not have a single cast expression. It has a cast expression for each target type. I think it's a pretty small detail and fixing it would require a lot of work.

zzcclp pushed a commit to zzcclp/spark that referenced this pull request Jul 28, 2016
@petermaxlee@cloud-fan
[SPARK-16730][SQL] Implement function aliases for type casts
11d427c
## What changes were proposed in this pull request?
Spark 1.x supports using the Hive type name as function names for doing casts, e.g.
```sql
SELECT int(1.0);
SELECT string(2.0);
```
The above query would work in Spark 1.x because Spark 1.x fail back to Hive for unimplemented functions, and break in Spark 2.0 because the fall back was removed.
This patch implements function aliases using an analyzer rule for the following cast functions:
- boolean
- tinyint
- smallint
- int
- bigint
- float
- double
- decimal
- date
- timestamp
- binary
- string
## How was this patch tested?
Added end-to-end tests in SQLCompatibilityFunctionSuite.
Author: petermaxlee <petermaxlee@gmail.com>
Closesapache#14364 from petermaxlee/SPARK-16730-2.
zzcclp pushed a commit to zzcclp/spark that referenced this pull request Jul 28, 2016
@petermaxlee@cloud-fan
[SPARK-16730][SQL] Implement function aliases for type casts
2d56a21
## What changes were proposed in this pull request?
Spark 1.x supports using the Hive type name as function names for doing casts, e.g.
```sql
SELECT int(1.0);
SELECT string(2.0);
```
The above query would work in Spark 1.x because Spark 1.x fail back to Hive for unimplemented functions, and break in Spark 2.0 because the fall back was removed.
This patch implements function aliases using an analyzer rule for the following cast functions:
- boolean
- tinyint
- smallint
- int
- bigint
- float
- double
- decimal
- date
- timestamp
- binary
- string
## How was this patch tested?
Added end-to-end tests in SQLCompatibilityFunctionSuite.
Author: petermaxlee <petermaxlee@gmail.com>
Closesapache#14364 from petermaxlee/SPARK-16730-2.
(cherry picked from commit 11d427c)
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
@cloud-fan

cloud-fan commented Jul 28, 2016

Copy link
Copy Markdown
Contributor

thanks, merging to master and 2.0!

@petermaxlee

petermaxlee commented Jul 28, 2016

Copy link
Copy Markdown
ContributorAuthor

Great. Thanks for merging.

Sign up for freeto 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.

4 participants

@petermaxlee@SparkQA@cloud-fan@hvanhovell