Skip to content

Conversation

@wangyum
Copy link
Member

@wangyum wangyum commented Jul 22, 2019

What changes were proposed in this pull request?

This PR change CalendarIntervalType's readable string representation from calendarinterval to interval.

How was this patch tested?

Existing UT

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Please update the title. Changing name is more important than adding simpleString.

@SparkQA
Copy link

SparkQA commented Jul 22, 2019

Test build #107989 has finished for PR 25225 at commit e84fce8.

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

@wangyum wangyum changed the title [SPARK-28469][SQL] Add simpleString for CalendarIntervalType [SPARK-28469][SQL] Change CalendarIntervalType's readable string representation from calendarinterval to interval Jul 22, 2019
@wangyum
Copy link
Member Author

wangyum commented Jul 22, 2019

retest this please

@dongjoon-hyun dongjoon-hyun self-requested a review July 22, 2019 07:27
@dongjoon-hyun
Copy link
Member

Thank you for updating.

@dongjoon-hyun
Copy link
Member

cc @gatorsmile

@SparkQA
Copy link

SparkQA commented Jul 22, 2019

Test build #107994 has finished for PR 25225 at commit e84fce8.

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

@dongjoon-hyun
Copy link
Member

I'm +1 for this changes.

@dongjoon-hyun
Copy link
Member

Although PostgreSQL has several variants. In Spark side, we have no such things. So, this will not be misleading. How do you think about this PR, @gatorsmile ?

postgres=# CREATE TABLE T AS SELECT INTERVAL '1' YEAR A, INTERVAL '1' MONTH B, INTERVAL '1' DAY C, INTERVAL '1' HOUR D, INTERVAL '1' MINUTE E, INTERVAL '1' SECOND F, INTERVAL '1 day';
SELECT 1
postgres=# \d T;
                      Table "public.t"
  Column  |      Type       | Collation | Nullable | Default
----------+-----------------+-----------+----------+---------
 a        | interval year   |           |          |
 b        | interval month  |           |          |
 c        | interval day    |           |          |
 d        | interval hour   |           |          |
 e        | interval minute |           |          |
 f        | interval second |           |          |
 interval | interval        |           |          |

@maropu
Copy link
Member

maropu commented Jul 23, 2019

+1, too.

fyi: the interval name seems to be specified by SQL-92:

Compatibility: The following types (or spellings thereof) are specified by SQL: bigint, bit, bit varying, boolean, char, character varying, character, varchar, date, double precision, integer, interval, numeric, decimal, real, smallint, time (with or without time zone), timestamp (with or without time zone), xml.

cited from https://www.postgresql.org/docs/9.2/datatype.html

@dongjoon-hyun
Copy link
Member

Yep. I think it's okay by itself. Thank you for the pointer.

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

Merged to master. Thank you, @wangyum and @maropu .
cc @gatorsmile and @cloud-fan

@HyukjinKwon
Copy link
Member

+1 too

@wangyum wangyum deleted the SPARK-28469 branch July 23, 2019 04:44
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