Skip to content

Strict INTERVAL literal syntax #23157

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

Closed
wants to merge 1 commit into from
Closed

Strict INTERVAL literal syntax #23157

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 12 additions & 6 deletions core/trino-grammar/src/main/antlr4/io/trino/grammar/sql/SqlBase.g4
View file
Open in desktop
Copy link
Member

@hashhar hashhar Sep 4, 2024

Choose a reason for hiding this comment

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

First thing I notice - the grammar in ANSI SQL matches what we had earlier. The restrictions on what combination of from and to are valid is mentioned in the "Syntax Rules" section.

Original file line number Diff line number Diff line change
Expand Up @@ -720,11 +720,12 @@ booleanValue
;

interval
: INTERVAL sign=(PLUS | MINUS)? string from=intervalField (TO to=intervalField)?
;

intervalField
: YEAR | MONTH | DAY | HOUR | MINUTE | SECOND
: INTERVAL sign=(PLUS | MINUS)? string from=YEAR (TO to=MONTH)?
| INTERVAL sign=(PLUS | MINUS)? string from=MONTH
| INTERVAL sign=(PLUS | MINUS)? string from=DAY (TO to=(HOUR | MINUTE | SECOND))?
| INTERVAL sign=(PLUS | MINUS)? string from=HOUR (TO to=(MINUTE | SECOND))?
| INTERVAL sign=(PLUS | MINUS)? string from=MINUTE (TO to=SECOND)?
| INTERVAL sign=(PLUS | MINUS)? string from=SECOND
Comment on lines +723 to +728
Copy link
Member

@hashhar hashhar Sep 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The rules basically say:

  • If TO is specified then fromSHALL NOT be MONTH
  • If TO is specified then from SHALL be more significant than to.
  • If from=YEAR then to=MONTH SHALL be the only valid option.

From the above the only valid combinations are:

from=YEAR
from=YEAR TO to=MONTH
from=MONTH
from=DAY
from=DAY TO to=(HOUR | MINUTE | SECOND)
from=HOUR
from=HOUR TO to=(MINUTE | SECOND)
from=MINUTE
from=MINUTE TO to=SECOND
from=SECOND

The changes seem to match this.

And significance is defined in the order YEAR > MONTH > DAY > HOUR > MINUTE > SECOND.

;

normalForm
Expand All @@ -733,7 +734,12 @@ normalForm

type
: ROW '(' rowField (',' rowField)* ')' #rowType
| INTERVAL from=intervalField (TO to=intervalField)? #intervalType
| INTERVAL from=YEAR (TO to=MONTH)? #yearMonthIntervalDataType
| INTERVAL from=MONTH #yearMonthIntervalDataType
| INTERVAL from=DAY (TO to=(HOUR | MINUTE | SECOND))? #dayTimeIntervalDataType
| INTERVAL from=HOUR (TO to=(MINUTE | SECOND))? #dayTimeIntervalDataType
| INTERVAL from=MINUTE (TO to=MINUTE)? #dayTimeIntervalDataType
| INTERVAL from=SECOND #dayTimeIntervalDataType
Comment on lines +737 to +742
Copy link
Member

@hashhar hashhar Sep 4, 2024

Choose a reason for hiding this comment

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

why is the sign part missing here?

Copy link
Member

@hashhar hashhar Sep 4, 2024

Choose a reason for hiding this comment

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

Also since you've changed the type names here I guess AstBuilder also needs to be updated.

| base=TIMESTAMP ('(' precision = typeParameter ')')? (WITHOUT TIME ZONE)? #dateTimeType
| base=TIMESTAMP ('(' precision = typeParameter ')')? WITH TIME ZONE #dateTimeType
| base=TIME ('(' precision = typeParameter ')')? (WITHOUT TIME ZONE)? #dateTimeType
Expand Down
18 changes: 18 additions & 0 deletions core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalDayTime.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,24 @@ public void testLiterals()

assertThatThrownBy(assertions.expression("INTERVAL '--12 -10' DAY TO HOUR")::evaluate)
.hasMessage("line 1:12: '--12 -10' is not a valid INTERVAL literal");

assertThatThrownBy(assertions.expression("INTERVAL '12' DAY TO YEAR")::evaluate)
.hasMessage("line 1:33: mismatched input 'YEAR'. Expecting: 'HOUR', 'MINUTE', 'SECOND'");

assertThatThrownBy(assertions.expression("INTERVAL '12' DAY TO MONTH")::evaluate)
.hasMessage("line 1:33: mismatched input 'MONTH'. Expecting: 'HOUR', 'MINUTE', 'SECOND'");

assertThatThrownBy(assertions.expression("INTERVAL '12' DAY TO DAY")::evaluate)
.hasMessage("line 1:33: mismatched input 'DAY'. Expecting: 'HOUR', 'MINUTE', 'SECOND'");

assertThatThrownBy(assertions.expression("INTERVAL '12' HOUR TO HOUR")::evaluate)
.hasMessage("line 1:34: mismatched input 'HOUR'. Expecting: 'MINUTE', 'SECOND'");

assertThatThrownBy(assertions.expression("INTERVAL '12' MINUTE TO MINUTE")::evaluate)
.hasMessage("line 1:36: mismatched input 'MINUTE'. Expecting: 'SECOND'");

assertThatThrownBy(assertions.expression("INTERVAL '12' SECOND TO SECOND")::evaluate)
.hasMessageContaining("line 1:33: mismatched input 'TO'.");
}

private static SqlIntervalDayTime interval(int day, int hour, int minute, int second, int milliseconds)
Expand Down
6 changes: 6 additions & 0 deletions core/trino-main/src/test/java/io/trino/operator/scalar/interval/TestIntervalYearMonth.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,12 @@ public void testLiterals()

assertThatThrownBy(assertions.expression("INTERVAL '--124--30' YEAR TO MONTH")::evaluate)
.hasMessage("line 1:12: '--124--30' is not a valid INTERVAL literal");

assertThatThrownBy(assertions.expression("INTERVAL '124' YEAR TO YEAR"):: evaluate)
.hasMessage("line 1:35: mismatched input 'YEAR'. Expecting: 'MONTH'");

assertThatThrownBy(assertions.expression("INTERVAL '124' MONTH TO MONTH"):: evaluate)
.hasMessageContaining("line 1:33: mismatched input 'TO'.");
Comment on lines +87 to +88
Copy link
Member

@hashhar hashhar Sep 4, 2024

Choose a reason for hiding this comment

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

I don't see how this is better than the older behaviour?

}

private static SqlIntervalYearMonth interval(int year, int month)
Expand Down
4 changes: 0 additions & 4 deletions core/trino-main/src/test/java/io/trino/sql/analyzer/TestAnalyzer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3727,10 +3727,6 @@ public void testLiteral()
.hasErrorCode(INVALID_LITERAL);
assertFails("SELECT INTERVAL '12.1' DAY TO SECOND")
.hasErrorCode(INVALID_LITERAL);
assertFails("SELECT INTERVAL '12' YEAR TO DAY")
.hasErrorCode(INVALID_LITERAL);
assertFails("SELECT INTERVAL '12' SECOND TO MINUTE")
.hasErrorCode(INVALID_LITERAL);

// json
assertFails("SELECT JSON ''")
Expand Down
24 changes: 18 additions & 6 deletions core/trino-parser/src/main/java/io/trino/sql/parser/AstBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,7 +306,6 @@
import io.trino.sql.tree.ZeroOrOneQuantifier;
import org.antlr.v4.runtime.ParserRuleContext;
import org.antlr.v4.runtime.Token;
import org.antlr.v4.runtime.tree.ParseTree;
import org.antlr.v4.runtime.tree.TerminalNode;

import java.util.ArrayDeque;
Expand Down Expand Up @@ -3308,9 +3307,8 @@ public Node visitInterval(SqlBaseParser.IntervalContext context)
Optional.ofNullable(context.sign)
.map(AstBuilder::getIntervalSign)
.orElse(IntervalLiteral.Sign.POSITIVE),
getIntervalFieldType((Token) context.from.getChild(0).getPayload()),
getIntervalFieldType(context.from),
Optional.ofNullable(context.to)
.map((x) -> x.getChild(0).getPayload())
.map(Token.class::cast)
.map(AstBuilder::getIntervalFieldType));
}
Expand Down Expand Up @@ -3397,11 +3395,25 @@ public Node visitTypeParameter(SqlBaseParser.TypeParameterContext context)
}

@Override
public Node visitIntervalType(SqlBaseParser.IntervalTypeContext context)
public Node visitYearMonthIntervalDataType(SqlBaseParser.YearMonthIntervalDataTypeContext context)
{
String from = context.from.getText();
String to = Optional.ofNullable((ParserRuleContext) context.to)
.map(ParseTree::getText)
String to = Optional.ofNullable(context.to)
.map(Token::getText)
.orElse(from);

return new IntervalDayTimeDataType(
getLocation(context),
IntervalDayTimeDataType.Field.valueOf(from.toUpperCase(ENGLISH)),
IntervalDayTimeDataType.Field.valueOf(to.toUpperCase(ENGLISH)));
}

@Override
public Node visitDayTimeIntervalDataType(SqlBaseParser.DayTimeIntervalDataTypeContext context)
{
String from = context.from.getText();
String to = Optional.ofNullable(context.to)
.map(Token::getText)
.orElse(from);

return new IntervalDayTimeDataType(
Expand Down