Skip to content

[SPARK-29855][SQL] typed literals with negative sign with proper result or exception - #26479

Closed
yaooqinn wants to merge 2 commits into
apache:masterfrom
yaooqinn:SPARK-29855
Closed

[SPARK-29855][SQL] typed literals with negative sign with proper result or exception#26479
yaooqinn wants to merge 2 commits into
apache:masterfrom
yaooqinn:SPARK-29855

Conversation

@yaooqinn

@yaooqinnyaooqinn commented Nov 12, 2019

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

-- !query 83select-integer'7'-- !query 83 schema
struct<7:int>-- !query 83 output7-- !query 86select-date'1999-01-01'-- !query 86 schema
struct<DATE'1999-01-01':date>-- !query 86 output1999-01-01-- !query 87select-timestamp'1999-01-01'-- !query 87 schema
struct<TIMESTAMP('1999-01-01 00:00:00'):timestamp>-- !query 87 output1999-01-0100:00:00

the integer should be -7 and the date and timestamp results are confusing which should throw exceptions

Why are the changes needed?

bug fix

Does this PR introduce any user-facing change?

NO

How was this patch tested?

ADD UTs

@yaooqinn

yaooqinn commented Nov 12, 2019

Copy link
Copy Markdown
MemberAuthor

cc @cloud-fan@maropu@HyukjinKwon@MaxGekk, thanks a lot

cloud-fan
cloud-fan reviewed Nov 12, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated
val value = string(ctx.STRING)
val valueType = ctx.identifier.getText.toUpperCase(Locale.ROOT)
val negativeSign = Option(ctx.negativeSign).map(_.getText).getOrElse("")
val isNegative = if (negativeSign == "-") true else false

@cloud-fancloud-fanNov 12, 2019

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.

is it simply val isNegative = ctx.negativeSign != null?

@cloud-fan

cloud-fan commented Nov 12, 2019

Copy link
Copy Markdown
Contributor

looks reasonable to me. Is it a common behavior in other DBs like pgsql?

MaxGekk
MaxGekk reviewed Nov 12, 2019
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala Outdated
val value = string(ctx.STRING)
val valueType = ctx.identifier.getText.toUpperCase(Locale.ROOT)
val negativeSign = Option(ctx.negativeSign).map(_.getText).getOrElse("")
val isNegative = if (negativeSign == "-") true else false

@MaxGekkMaxGekkNov 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
valisNegative=if (negativeSign =="-") trueelsefalse
valisNegative=ctx.negativeSign !=null&& ctx.negativeSign.getText=="-"

@MaxGekkMaxGekkNov 12, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

and remove val negativeSign

@cloud-fancloud-fanNov 12, 2019

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.

do we really need to check ctx.negativeSign.getText? the parser rule is MINUS? ...

@yaooqinnyaooqinnNov 12, 2019

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

negativeSign is used in error message, but I think @cloud-fan 's point is right, we need not to check ctx.negativeSign.getText. I'd do some tests to verify this.

@SparkQA

SparkQA commented Nov 12, 2019

Copy link
Copy Markdown

Test build #113615 has finished for PR 26479 at commit 1d04cb3.

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

@yaooqinn

yaooqinn commented Nov 12, 2019

Copy link
Copy Markdown
MemberAuthor

looks reasonable to me. Is it a common behavior in other DBs like pgsql?

yes it right the pg behavior

cloud-fan
cloud-fan reviewed Nov 12, 2019
View reviewed changes
Comment threadsql/core/src/test/resources/sql-tests/inputs/literals.sql
select '1' year to second;
select interval 1 year '2-1' year to month;
select 1 year '2-1' year to month;
SET spark.sql.ansi.enabled=false;

@cloud-fancloud-fanNov 12, 2019

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.

not related to this PR, but it's really annoying to duplicate the test cases with ansi mode on and off.

I'm wondering if we can introduce a -- import abc.sql directive so that we can easily import test cases in ansi/interval.sql, with several ansi specific test cases. cc @maropu

@maropumaropuNov 13, 2019

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see. That's certainly annoying. I'll try to make a pr later for that approach.

@SparkQA

SparkQA commented Nov 12, 2019

Copy link
Copy Markdown

Test build #113617 has finished for PR 26479 at commit 8f524c3.

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

@HyukjinKwon

HyukjinKwon commented Nov 12, 2019

Copy link
Copy Markdown
Member

Merged to master.

@maropumaropu mentioned this pull request Nov 13, 2019
Closed
cloud-fan pushed a commit that referenced this pull request Nov 14, 2019
@maropu@cloud-fan
[SPARK-29873][SQL][TESTS] Support--importdirective to load querie…
b5a02d3
…s from another test case in SQLQueryTestSuite
### What changes were proposed in this pull request?
This pr is to support `--import` directive to load queries from another test case in SQLQueryTestSuite.
This fix comes from the cloud-fan suggestion in #26479 (comment)
### Why are the changes needed?
This functionality might reduce duplicate test code in `SQLQueryTestSuite`.
### Does this PR introduce any user-facing change?
No.
### How was this patch tested?
Run `SQLQueryTestSuite`.
Closes#26497 from maropu/ImportTests.
Authored-by: Takeshi Yamamuro <yamamuro@apache.org>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@maropumaropumaropu left review comments
@MaxGekkMaxGekkMaxGekk left review comments
@cloud-fancloud-fancloud-fan approved these changes
@HyukjinKwonHyukjinKwonHyukjinKwon approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@yaooqinn@cloud-fan@SparkQA@HyukjinKwon@maropu@MaxGekk@dongjoon-hyun