Skip to content
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

[SPARK-29629][SQL] Support typed integer literal expression #26291

Closed
wants to merge 3 commits into from
Closed

[SPARK-29629][SQL] Support typed integer literal expression #26291

wants to merge 3 commits into from

Conversation

yaooqinn
Copy link
Member

What changes were proposed in this pull request?

postgres=# select date '2001-09-28' + integer '7';
  ?column?
------------
 2001-10-05
(1 row)postgres=# select integer '7';
 int4
------
    7
(1 row)

Add support for typed integer literal expression from postgreSQL.

Why are the changes needed?

SPARK-27764 Feature Parity between PostgreSQL and Spark

Does this PR introduce any user-facing change?

support typed integer lit in SQL

How was this patch tested?

add uts

@SparkQA
Copy link

SparkQA commented Oct 29, 2019

Test build #112828 has finished for PR 26291 at commit b453a2e.

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

@yaooqinn
Copy link
Member Author

retest this please

MaxGekk
MaxGekk reviewed Oct 29, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated
ex.setStackTrace(e.getStackTrace)
throw ex
}
Literal(i)
Copy link
Member

Choose a reason for hiding this comment

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

Could you replace it by Literal(i, IntegerType), so, Literal.apply() doesn't have to pattern match i to infer IntegerType.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok, that is better.

MaxGekk
MaxGekk reviewed Oct 29, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated
@@ -1799,6 +1799,16 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
case "X" =>
val padding = if (value.length % 2 != 0) "0" else ""
Literal(DatatypeConverter.parseHexBinary(padding + value))
case "INTEGER" =>
val i = try {
Integer.parseInt(value)
Copy link
Member

Choose a reason for hiding this comment

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

nit: Scalish style value.toInt.

MaxGekk
MaxGekk reviewed Oct 29, 2019
View reviewed changes
sql/core/src/test/resources/sql-tests/inputs/datetime.sql Outdated
@@ -36,3 +36,5 @@ select date '2001-10-01' - 7;
select date '2001-10-01' - date '2001-09-28';
select date'2020-01-01' - timestamp'2019-10-06 10:11:12.345678';
select timestamp'2019-10-06 10:11:12.345678' - date'2020-01-01';
select date '2001-09-28' + integer '7';
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this is necessary test. datetime.sql aims to test date-time related functionality. And date +/- integer has been already covered by https://github.com/apache/spark/pull/26291/files#diff-e7a79cbcd5411e8891cee65c286c2177L33

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for your review. updated

MaxGekk
MaxGekk approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for jenkins

@SparkQA
Copy link

SparkQA commented Oct 29, 2019

Test build #112831 has finished for PR 26291 at commit b453a2e.

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

@SparkQA
Copy link

SparkQA commented Oct 29, 2019

Test build #112834 has finished for PR 26291 at commit cdce795.

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

@SparkQA
Copy link

SparkQA commented Oct 29, 2019

Test build #112836 has finished for PR 26291 at commit 1960b29.

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

HyukjinKwon
HyukjinKwon reviewed Oct 30, 2019
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
@@ -1799,6 +1799,16 @@ class AstBuilder(conf: SQLConf) extends SqlBaseBaseVisitor[AnyRef] with Logging
case "X" =>
val padding = if (value.length % 2 != 0) "0" else ""
Literal(DatatypeConverter.parseHexBinary(padding + value))
case "INTEGER" =>
Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, how many such typed literals are there left?

@HyukjinKwon
Copy link
Member

Merged to master.

@cloud-fan
Copy link
Contributor

Shall we revert it? It seems weird to me to write int literals with INTEGER '1' instead of 1. Now we don't care too much about pgsql compatibility, and we need to make sure these new features are reasonable.

cc @gatorsmile @xuanyuanking @gengliangwang

@yaooqinn
Copy link
Member Author

Postgre +1 Presto +1 MySQL -1, ANSI Standard (Seems -1)

@gengliangwang
Copy link
Member

gengliangwang commented Dec 19, 2019 •
edited

+1 with @cloud-fan
In PostgreSQL, all the data types in https://www.postgresql.org/docs/9.1/datatype-numeric.html should be supported. E.g. smallint '1', float '2.3'
It is wired to support 'integer' only.
I am going to revert this one.
Also cc @HyukjinKwon

@xuanyuanking
Copy link
Member

+1 for reverting, the Jira SPARK-30125 should be reused, then all the reverting PRs are gathered together.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MaxGekk MaxGekk MaxGekk approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees
No one assigned
Labels
SQL
Projects
None yet
Milestone
No milestone
8 participants
@yaooqinn @SparkQA @HyukjinKwon @cloud-fan @gengliangwang @xuanyuanking @MaxGekk @dongjoon-hyun