Strict INTERVAL literal syntax

[takezoe]

Description

The current INTERVAL literal syntax allows below but it should be rejected:

INTERVAL '124' YEAR TO YEAR

Semantically invalid literals like below becomes an error at the analysis phase but the error message is not clear (See #23002). Anyway, it's better to reject at the syntax level.

INTERVAL '124' MONTH TO YEAR

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.
( ) Release notes are required, with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

[hashhar]

Thanks for the PR, I'm adding syntax-needs-review while I check what the SQL standard has to say about this (e.g. future incompatibilities we may run into).

[ebyhr]

@hashhar FYI, this syntax is documented in 10.1 <interval qualifier> of SQL:2016. Especially, Syntax Rules 1) and 2) are related to this change. 1) explains ordering of YEAR, MONTH, DAY, HOUR, MINUTE, and SECOND. 2) explains MONTH & YEAR cases.

[takezoe]

The second case is anyway rejected at the analysis phase currently, so the only first example (specifying the same unit both for FROM and TO) could be an incompatibility.

[hashhar]

Sorry it took me so long.

The grammar changes seem in agreement with spec. The only caveat being that the spec specifies the restriction in "syntax rules" with language using "SHALL". I don't know what this implies. In general do we implement the checks under "syntax rules" in analyzer or in grammar itself? @martint to answer this.

I've yet to look at tests, will try to do by end of day.

[hashhar]

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.

[hashhar]

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.

[hashhar]

why is the sign part missing here?

[hashhar]

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

[hashhar]

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

[martint]

In general do we implement the checks under "syntax rules" in analyzer or in grammar itself? @martint to answer this.

It depends on each case. Some things are not possible to constrain at the grammar level. For others, it's easier to produce more user-friendly error messages by enforcing the constraints during analysis.

This one is in a grey area. I would argue that at the grammar level we just want to support the general shape (to / from field or just a single field) and leave the validation of the specific combinations to the analysis phase. We have to enforce the validity during analysis anyway, since the AST allows for any combination of to/from fields. The format of the interval string also depends on the combination of from/to fields, and that has to be validated during analysis, too.

[hashhar]

So it seems #23002 is the better direction to go for then.

[takezoe]

As far as I have checked, some major SQL engines seem to check this at the syntax level. However, choice in Trino is ours. I don't have objection as long as invalid literals are rejected with user-friendly error message.

[takezoe]

I will close this since we seem to prefer to treat this as a semantic error. Thank you for all comments here. I will continue working on #23002.