Add GRACE PERIOD to SHOW CREATE MATERIALIZED VIEW #27529
Conversation
piotrrzysko
force-pushed
the
show_create_grace_period
branch
from
8701367 to
7e3c950
Compare
| public static IntervalLiteral formatDayTimeInterval(Duration duration) | ||
| { | ||
| long millis = duration.toMillis(); | ||
| Period period = new Period(Math.abs(millis)).normalizedStandard(PeriodType.dayTime()); |
There was a problem hiding this comment.
add
IntervalLiteral.Sign sign = millis < 0 ? NEGATIVE : POSITIVE;so that abs is not scary
|
|
||
| PeriodFormatterBuilder builder = new PeriodFormatterBuilder(); | ||
| PeriodFormatterBuilder builder = new PeriodFormatterBuilder() | ||
| .printZeroIfSupported(); |
| break; | ||
| } | ||
| builder.appendLiteral(":"); | ||
| builder.minimumPrintedDigits(2); |
| @Test | ||
| void testDayTimeIntervalRoundTrip() | ||
| { | ||
| testDayTimeIntervalRoundTrip("0", SECOND, Optional.empty()); |
There was a problem hiding this comment.
i'd rather have this take SQL interval string as input to test roundtrip.
That would be testing at the level at which things must be stable, and without adding too many layers IMO.
| if (period.getDays() > 0) { | ||
| startField = DAY; | ||
| if (period.getSeconds() > 0 || period.getMillis() > 0) { | ||
| endField = Optional.of(SECOND); | ||
| value = INTERVAL_DAY_SECOND_FORMATTER.print(period); | ||
| } | ||
| else if (period.getMinutes() > 0) { | ||
| endField = Optional.of(MINUTE); | ||
| value = INTERVAL_DAY_MINUTE_FORMATTER.print(period); | ||
| } | ||
| else if (period.getHours() > 0) { | ||
| endField = Optional.of(HOUR); | ||
| value = INTERVAL_DAY_HOUR_FORMATTER.print(period); | ||
| } | ||
| else { | ||
| endField = Optional.empty(); | ||
| value = INTERVAL_DAY_FORMATTER.print(period); | ||
| } | ||
| } | ||
| else if (period.getHours() > 0) { | ||
| startField = HOUR; | ||
| if (period.getSeconds() > 0 || period.getMillis() > 0) { | ||
| endField = Optional.of(SECOND); | ||
| value = INTERVAL_HOUR_SECOND_FORMATTER.print(period); | ||
| } | ||
| else if (period.getMinutes() > 0) { | ||
| endField = Optional.of(MINUTE); | ||
| value = INTERVAL_HOUR_MINUTE_FORMATTER.print(period); | ||
| } | ||
| else { | ||
| endField = Optional.empty(); | ||
| value = INTERVAL_HOUR_FORMATTER.print(period); | ||
| } | ||
| } | ||
| else if (period.getMinutes() > 0) { | ||
| startField = MINUTE; | ||
| if (period.getSeconds() > 0 || period.getMillis() > 0) { | ||
| endField = Optional.of(SECOND); | ||
| value = INTERVAL_MINUTE_SECOND_FORMATTER.print(period); | ||
| } | ||
| else { | ||
| endField = Optional.empty(); | ||
| value = INTERVAL_MINUTE_FORMATTER.print(period); | ||
| } | ||
| } | ||
| else { | ||
| startField = SECOND; | ||
| endField = Optional.empty(); | ||
| value = INTERVAL_SECOND_FORMATTER.print(period); | ||
| } |
There was a problem hiding this comment.
This looks terribly verbose, but i don't know how to improve it. ok to keep
There was a problem hiding this comment.
One option is to reuse the function we use for casts:
but then the output is verbose:
trino/core/trino-main/src/test/java/io/trino/type/TestIntervalDayTime.java
Lines 431 to 434 in 518b244
There was a problem hiding this comment.
Keep in mind this is coming: #27539
Thanks for the link!
i skimmed the PR and it looks mostly backwards compatible.
The code in this PR takes java time interval and formats it as a SQL literal, so it should work both before and after the same.
One option is to reuse the function we use for casts
Sounds much simpler. How would that look like @piotrrzysko ?
Would we still output CREATE MV statement that can be pasted back into CLI and run?
There was a problem hiding this comment.
A prototype: piotrrzysko@962fe50
I added tests that demonstrate the formatted result can be parsed back into the original input (expressed in milliseconds, since the original format is not retained).
The only downside I see in this approach, as mentioned earlier, is that the formatted values are not succinct (e.g., INTERVAL '1' HOUR becomes INTERVAL '0 01:00:00.000' DAY TO SECOND).
There was a problem hiding this comment.
We can't preserve original syntax (1 hour = 60 minutes and we don't distinguish)
I agree that INTERVAL '1' HOUR looks nicer than INTERVAL '0 01:00:00.000' DAY TO SECOND.
But less code looks nicer to me.
What I am worried is that "reuse the function we use for casts" may be "opportunistic reuse" -- reuse of something that does what we need today, but has different design constrains and so can change in the future.
Unless/until we have confirmation "the function we use for casts" is guaranteed to produce a string literal that we can put directly into a SQL, let's not use it.
|
please rebase |
3 participants
Description
This PR adds support for displaying the
GRACE PERIODclause in the output ofSHOW CREATE MATERIALIZED VIEW.Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text: