Skip to content

[SPARK-28495][SQL][FOLLOW-UP] Disallow conversions between timestamp and long in ASNI mode#25615

Closed
gengliangwang wants to merge 1 commit into
apache:masterfrom
gengliangwang:disallowTimeStampToLong
Closed

[SPARK-28495][SQL][FOLLOW-UP] Disallow conversions between timestamp and long in ASNI mode#25615
gengliangwang wants to merge 1 commit into
apache:masterfrom
gengliangwang:disallowTimeStampToLong

Conversation

@gengliangwang

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

Disallow conversions between timestamp type and long type in table insertion with ANSI store assignment policy.

Why are the changes needed?

In the PR #25581, timestamp type is allowed to be converted to long type, since timestamp type is represented by long type internally, and both legacy mode and strict mode allows the conversion.

After reconsideration, I think we should disallow it. As per ANSI SQL section "4.4.2 Characteristics of numbers":

A number is assignable only to sites of numeric type.

In PostgreSQL, the conversion between timestamp and long is also disallowed.

Does this PR introduce any user-facing change?

Conversion between timestamp and long is disallowed in table insertion with ANSI store assignment policy.

How was this patch tested?

Unit test

@gengliangwang

Copy link
Copy Markdown
Member Author

@cloud-fan @maropu Please help review this one.

@maropu

maropu commented Aug 29, 2019

Copy link
Copy Markdown
Member

The change looks reasonable to me. Just in case, I checked the pgSQL behaivour again;

postgres=# create table t (l bigint);
postgres=# insert into t values ('20190829 00:00:00'::timestamp);
2019-08-29 15:26:10.998 JST [4553] ERROR:  column "l" is of type bigint but expression is of type timestamp without time zone at character 23
2019-08-29 15:26:10.998 JST [4553] HINT:  You will need to rewrite or cast the expression.
2019-08-29 15:26:10.998 JST [4553] STATEMENT:  insert into t values ('20190829 00:00:00'::timestamp);
ERROR:  column "l" is of type bigint but expression is of type timestamp without time zone
LINE 1: insert into t values ('20190829 00:00:00'::timestamp);
                           

"Should not allow timestamp to long") { err =>
assert(err.contains("Cannot safely cast 'timestampToLong': TimestampType to LongType"))
}
}

@maropu maropu Aug 29, 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.

btw, we don't have any existing test for these cast case, right? (I just a bit worried about forgetting to add tests for the other cast cases in the ANSI mode...

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think so. Let's wait for the test result.

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.

yea, ok.

@SparkQA

SparkQA commented Aug 29, 2019

Copy link
Copy Markdown

Test build #109899 has finished for PR 25615 at commit 75e3d96.

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

@gengliangwang

Copy link
Copy Markdown
Member Author

retest this please.

@SparkQA

SparkQA commented Aug 29, 2019

Copy link
Copy Markdown

Test build #109901 has finished for PR 25615 at commit 75e3d96.

  • 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!

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants