Skip to content

[SQL] Miscellaneous SQL/DF expression changes. #6754

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 6 commits into from
Closed

[SQL] Miscellaneous SQL/DF expression changes. #6754

wants to merge 6 commits into from

Conversation

@rxin
Copy link
Contributor

@rxin rxin commented Jun 11, 2015

SPARK-8201 conditional function: if
SPARK-8205 conditional function: nvl
SPARK-8208 math function: ceiling
SPARK-8210 math function: degrees
SPARK-8211 math function: radians
SPARK-8219 math function: negative
SPARK-8216 math function: rename log -> ln
SPARK-8222 math function: alias power / pow
SPARK-8225 math function: alias sign / signum
SPARK-8228 conditional function: isnull
SPARK-8229 conditional function: isnotnull
SPARK-8250 string function: alias lower/lcase
SPARK-8251 string function: alias upper / ucase

@rxin
[SQL] Miscellaneous SQL/DF expression changes.
6b21a9b 
SPARK-8201 conditional function: if
SPARK-8205 conditional function: nvl
SPARK-8208 math function: ceiling
SPARK-8210 math function: degrees
SPARK-8211 math function: radians
SPARK-8219 math function: negative
SPARK-8216 math function: rename log -> ln
SPARK-8222 math function: alias power / pow
SPARK-8225 math function: alias sign / signum
SPARK-8228 conditional function: isnull
SPARK-8229 conditional function: isnotnull
SPARK-8250 string function: alias lower/lcase
SPARK-8251 string function: alias upper / ucase
@rxin
Copy link
Contributor Author

rxin commented Jun 11, 2015

cc @adrian-wang @chenghao-intel

@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34656 has finished for PR 6754 at commit 6b21a9b.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)
    • case class StringLength(child: Expression) extends UnaryExpression with ExpectsInputTypes

@adrian-wang
Copy link
Contributor

adrian-wang commented Jun 11, 2015

We are having problems with If.
The checkInputDataTypes method should enable type coercion.

adrian-wang
adrian-wang reviewed Jun 11, 2015
View reviewed changes
sql/core/src/test/scala/org/apache/spark/sql/MathExpressionsSuite.scala
Copy link
Contributor

@adrian-wang adrian-wang Jun 11, 2015

Choose a reason for hiding this comment

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

I agree that we need to add tests here since they are math expressions, but I think we'd better keep tests in this suite consistent. These tests added here are in the style of DataFrameFunctionsSuite.

Copy link
Contributor

@chenghao-intel chenghao-intel Jun 11, 2015

Choose a reason for hiding this comment

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

I think this change is a great idea, as it's more straightforward when reading the code, otherwise we probably need to jump back and forth.

Copy link
Contributor

@adrian-wang adrian-wang Jun 11, 2015

Choose a reason for hiding this comment

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

Sort of...

Copy link
Contributor Author

@rxin rxin Jun 11, 2015

Choose a reason for hiding this comment

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

I think it makes sense to put all math stuff in here, all string stuff into its own suite, etc

basically if we group expressions into files; we should also group test suites in the same way.

Copy link
Contributor

@adrian-wang adrian-wang Jun 11, 2015

Choose a reason for hiding this comment

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

That makes sense.

@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34668 has finished for PR 6754 at commit b201cd4.

  • This patch fails Scala style tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34670 has finished for PR 6754 at commit de827ac.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Log2(child: Expression)
    • case class StringLength(child: Expression) extends UnaryExpression with ExpectsInputTypes

@rxin
Copy link
Contributor Author

rxin commented Jun 11, 2015

I think I fixed "if". @cloud-fan can you take a look? Thanks.

@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34674 has finished for PR 6754 at commit 3c32bbc.

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

cloud-fan
cloud-fan reviewed Jun 11, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala Outdated
Copy link
Contributor

@cloud-fan cloud-fan Jun 11, 2015

Choose a reason for hiding this comment

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

Hive support promote types to String for If, but our findTightestCommonTypeOfTwo doesn't support it. There are some other expressions like Coalesce, CaseWhen need to promote types to String. #6551 tries to fix it but it's not complete. I'm wondering if hive has a specific rule about string type promotion that we can follow, or is it all about hive's implicit conversions?

Copy link
Contributor

@lianhuiwang lianhuiwang Jun 11, 2015

Choose a reason for hiding this comment

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

i think we can define new TypeConversion class for Coalesce, If, CaseWhen, and so on. because in Hive these udf function use same one type Conversion rule as https://github.com/apache/hive/blob/ac755ebe26361a4647d53db2a28500f71697b276/ql/src/java/org/apache/hadoop/hive/ql/udf/generic/GenericUDFUtils.java#L79.

Copy link
Contributor Author

@rxin rxin Jun 11, 2015

Choose a reason for hiding this comment

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

yup that's a good idea.

Copy link
Contributor Author

@rxin rxin Jun 11, 2015

Choose a reason for hiding this comment

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

I added a rule to promote to string just for If for this patch. We can generalize it later.

@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34707 has finished for PR 6754 at commit 2647067.

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

@SparkQA
Copy link

SparkQA commented Jun 11, 2015

Test build #34718 has finished for PR 6754 at commit 35fce15.

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

marmbrus
marmbrus reviewed Jun 11, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/HiveTypeCoercion.scala
Copy link
Contributor

@marmbrus marmbrus Jun 11, 2015

Choose a reason for hiding this comment

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

Why this special case? Also, I'd just use Ifinstead of makeCopy here and above. Make copy is nice when you are matching on different but structurally similar expression, but looses compile time checks for arguments.

Copy link
Contributor Author

@rxin rxin Jun 11, 2015

Choose a reason for hiding this comment

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

"if(null, true, false)" gets a nulltype.

Copy link
Contributor

@cloud-fan cloud-fan Jun 12, 2015

Choose a reason for hiding this comment

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

what about

case i @ If (pred, _, _) if pred.dataType == NullType =>
  i.copy(predicate = Literal.create(null, BooleanType))

Copy link
Contributor

@marmbrus marmbrus Jun 12, 2015

Choose a reason for hiding this comment

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

+1

@marmbrus
Copy link
Contributor

marmbrus commented Jun 11, 2015

LGTM

@rxin
Copy link
Contributor Author

rxin commented Jun 12, 2015

Thanks. Merging. I will submit something else to replace the copy.

@asfgit asfgit closed this in 337c16d Jun 12, 2015
@rxin rxin mentioned this pull request Jun 13, 2015
Closed
asfgit pushed a commit that referenced this pull request Jun 14, 2015
@rxin
[SPARK-8347][SQL] Add unit tests for abs.
a138953 
Also addressed code review feedback from #6754

Author: Reynold Xin <[email protected]>

Closes #6803 from rxin/abs and squashes the following commits:

d07beba [Reynold Xin] [SPARK-8347] Add unit tests for abs.
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@rxin @nemccarthy
[SQL] Miscellaneous SQL/DF expression changes.
b060836 
SPARK-8201 conditional function: if
SPARK-8205 conditional function: nvl
SPARK-8208 math function: ceiling
SPARK-8210 math function: degrees
SPARK-8211 math function: radians
SPARK-8219 math function: negative
SPARK-8216 math function: rename log -> ln
SPARK-8222 math function: alias power / pow
SPARK-8225 math function: alias sign / signum
SPARK-8228 conditional function: isnull
SPARK-8229 conditional function: isnotnull
SPARK-8250 string function: alias lower/lcase
SPARK-8251 string function: alias upper / ucase

Author: Reynold Xin <[email protected]>

Closes apache#6754 from rxin/expressions-misc and squashes the following commits:

35fce15 [Reynold Xin] Removed println.
2647067 [Reynold Xin] Promote to string type.
3c32bbc [Reynold Xin] Fixed if.
de827ac [Reynold Xin] Fixed style
b201cd4 [Reynold Xin] Removed if.
6b21a9b [Reynold Xin] [SQL] Miscellaneous SQL/DF expression changes.
nemccarthy pushed a commit to nemccarthy/spark that referenced this pull request Jun 19, 2015
@rxin @nemccarthy
[SPARK-8347][SQL] Add unit tests for abs.
cfe530b 
Also addressed code review feedback from apache#6754

Author: Reynold Xin <[email protected]>

Closes apache#6803 from rxin/abs and squashes the following commits:

d07beba [Reynold Xin] [SPARK-8347] Add unit tests for abs.
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.

7 participants

@rxin @SparkQA @adrian-wang @marmbrus @lianhuiwang @chenghao-intel @cloud-fan