-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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.
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
feat(sql): Support exclusive and half-open intervals in BETWEEN expressions #4779
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some feedback and suggestions.
Its good to add tests as you go to ensure these changes work. You could start with:
QueryModelTest
- to make sure clearing etc. works
SqlParserTest
- to test your additions, make sure it parses correctly.
public static final int INCLUSIVE_BETWEEN = 1; | ||
public static final int EXCLUSIVE_BETWEEN = 2; | ||
public static final int RIGHT_OPEN_BETWEEN = 3; | ||
public static final int LEFT_OPEN_BETWEEN = 4; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefix with BETWEEN i.e BETWEEN_INCLUSIVE
, BETWEEN_EXCLUSIVE
@@ -184,6 +189,8 @@ public class QueryModel implements Mutable, ExecutionModel, AliasTranslator, Sin | |||
private QueryModel updateTableModel; | |||
private TableToken updateTableToken; | |||
private ExpressionNode whereClause; | |||
// exclusive and inclusive with between | |||
private int betweenType = INCLUSIVE_BETWEEN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default should be 0 or -1 since not every query has a BETWEEN statement in it
@@ -433,6 +440,7 @@ public void clear() { | |||
explicitTimestamp = false; | |||
showKind = -1; | |||
sampleByOffset = ZERO_OFFSET; | |||
betweenType = INCLUSIVE_BETWEEN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default should be 0 or -1 since not every query has a BETWEEN statement in it
tok = tok(lexer, "'inclusive' 'exclusive' 'left' 'open' 'right'"); | ||
|
||
if(SqlKeywords.isInclusiveKeyword(tok)) { | ||
// for INCLUSIVE | ||
} else if(SqlKeywords.isExclusiveKeyword(tok)) { | ||
// for EXCLUSIVE | ||
} else if (SqlKeywords.isRightKeyword(tok)) { | ||
if (SqlKeywords.isOpenKeyword(tok)) { | ||
// for RIGHT OPEN | ||
} | ||
} else if (SqlKeywords.isLeftKeyword(tok)) { | ||
if (SqlKeywords.isOpenKeyword(tok)) { | ||
// for LEFT OPEN | ||
} | ||
} else { | ||
throw SqlException.$(lexer.lastTokenPosition(), "''inclusive' 'exclusive' 'left' 'open' 'right' expected"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good start here.
Check the else clause - a keyword is not required at this point, it could just be BETWEEN x AND y
which is would be equivalent to BETWEEN x AND y INCLUSIVE
.
For RIGHT OPEN and LEFT OPEN, you'll need to progress the lexer with methods like tok or optTok.
@@ -1195,6 +1208,14 @@ public static boolean isOnlyKeyword(CharSequence tok) { | |||
&& (tok.charAt(3) | 32) == 'y'; | |||
} | |||
|
|||
public static boolean isOpenKeyword(CharSequence tok) { | |||
return tok.length() == 9 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OPEN
only has 4 letters.
public void setBetweenType(int betweenType) { | ||
this.betweenType = betweenType; | ||
} | ||
|
||
@Override | ||
public void toSink(@NotNull CharSink<?> sink) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add to this to make sure the extra keywords (exclusive, right open etc.) are written out into the model when its printed.
If you step through the code with a debugger, you can quickly view the model using QueryModel.toString0()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't quite get this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We run some tests by parsing the SQL into a QueryModel and then printing this model out for comparison.
You can update toSink() to make it print out your new additions. This will help you to write tests to make sure your new parsing logic works as expected.
See SqlParserTest which has tests asserting against the QueryModel.toSink() output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, while testing I am getting this error repeatedly.
The type jdk.internal.math.FDBigInteger is not accessible
and other errors related to jdk.internal
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you give more context? Like a stack trace or info about where it is being thrown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used this and tried to write tests and update the parser but I can't test because of these errors. Should I build the project again, but I'll lose all the changes? How do I proceed? Do you have any suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure wy this is being thrown. Perhaps its related to Java versions, Coretto 17 should work.
You could commit and push, and then delete the directory and re-clone it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, I did reclone it, but it still shows the same errors, I think I need to clone it from the main repo, since there are only 60-70 lines of changes I can redo them after cloning the main repo. But I am not sure about this pr, do I need to create a new one and delete this one? Also, I am sorry for not doing anything in the past week as I was busy with my school exams. Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do whatever is most convenient, and no problem about the delays :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will close this pr and create a new one!!!
No description provided.