Improve handling of negative GRACE PERIOD #27517
Open
+19
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wendigo
approved these changes
Dec 1, 2025
martint
requested changes
Dec 1, 2025
| if (context.GRACE() != null) { | ||
| gracePeriod = Optional.of((IntervalLiteral) visit(context.interval())); | ||
| IntervalLiteral interval = (IntervalLiteral) visit(context.interval()); | ||
| if (interval.getSign() == IntervalLiteral.Sign.NEGATIVE) { |
Member
There was a problem hiding this comment.
This should be done in the semantic analyzer, not in the AST builder.
Member
Author
There was a problem hiding this comment.
Thanks, fixed. I think I understand the reason. Just out of curiosity: if we had an unsigned interval type and used it for GRACE PERIOD, would that be a syntax error then?
Member
There was a problem hiding this comment.
Correct. But that would be handled in the grammar. Syntax checks in the AST builder should be limited to extreme cases that cannot handled easily via antlr4 rules.
Previously, providing a negative grace period resulted in a low-level internal exception: Caused by: java.lang.IllegalArgumentException: gracePeriod cannot be negative: Optional[PT-1H] at com.google.common.base.Preconditions.checkArgument(Preconditions.java:217) at io.trino.metadata.MaterializedViewDefinition.<init>(MaterializedViewDefinition.java:49)
piotrrzysko
force-pushed
the
negative_grace_period
branch
from
2a42eb7 to
0dbcd6e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Previously, providing a negative grace period resulted in a low-level internal exception:
Additional context and related issues
Release notes
(x) 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: