xiedeyantu commented on code in PR #4797:
URL: https://github.com/apache/calcite/pull/4797#discussion_r2834053490
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -3311,16 +3311,17 @@ static void checkOverlaps(OverlapChecker c) {
* SqlOverlapsOperator does not reject some illegal comparisons (e.g., TIME
vs DATE)</a>. */
@Test void testNegativePeriodOperators() {
final String containsError = "Supported form\\(s\\): "
- + "'\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\) CONTAINS "
- + "\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\)'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\) CONTAINS "
- + "\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\)'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\) CONTAINS "
- + "\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\)'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\) CONTAINS "
- + "\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\)'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\) CONTAINS
<DATE/TIME/TIMESTAMP>'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\) CONTAINS
<DATE/TIME/TIMESTAMP>'";
+ + "'\\(<DT>, <DT>\\) CONTAINS "
Review Comment:
Would it be clearer to put them in one line?
```
final String containsError = "Supported form\\(s\\): "
+ "'\\(<DT>, <DT>\\) CONTAINS \\(<DT>, <DT>\\)'\\n"
+ "'\\(<DT>, <DT>\\) CONTAINS \\(<DT>, <INTERVAL>\\)'\\n"
+ "'\\(<DT>, <INTERVAL>\\) CONTAINS \\(<DT>, <DT>\\)'\\n"
+ "'\\(<DT>, <INTERVAL>\\) CONTAINS \\(<DT>, <INTERVAL>\\)'\\n"
+ "'\\(<DT>, <DT>\\) CONTAINS <DT>'\\n"
+ "'\\(<DT>, <INTERVAL>\\) CONTAINS <DT>'\\n"
+ "Where 'DT' is one of 'DATE', 'TIME', or 'TIMESTAMP', the same for
all arguments.";
```
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlOverlapsOperator.java:
##########
@@ -77,8 +79,20 @@ void arg(SqlWriter writer, SqlCall call, int leftPrec, int
rightPrec, int i) {
return SqlOperandCountRanges.of(2);
}
+ @Override public @Nullable String getSignatureTemplate(final int
operandsCount) {
Review Comment:
Sorry, I didn't notice the override. But it looks good now.
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlOverlapsOperator.java:
##########
@@ -128,9 +142,12 @@ void arg(SqlWriter writer, SqlCall call, int leftPrec, int
rightPrec, int i) {
final SqlSingleOperandTypeChecker rightChecker;
switch (kind) {
case CONTAINS:
+ // A ternary call of the form (a, b) CONTAINS c
rightChecker = OperandTypes.PERIOD_OR_DATETIME;
break;
default:
+ // Always a quaternary call of the form (a, b) OVERLAPS (c, d)
+ // (or one of the other 7 kinds of period operators)
Review Comment:
Perhaps I'm not familiar enough with these operators. Can the seven
operators mentioned here be linked to the actual enumeration code? Of course, I
think the current comments are already quite clear, and those familiar with
this part should understand them easily. We can leave them as they are.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -3347,14 +3348,15 @@ static void checkOverlaps(OverlapChecker c) {
+ "CONTAINS <TIMESTAMP\\(0\\)>'\\. "
+ containsError, false);
final String overlapsError = "Supported form\\(s\\): "
- + "'\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\) OVERLAPS "
- + "\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\)'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\) OVERLAPS "
- + "\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\)'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\) OVERLAPS "
- + "\\(<DATE/TIME/TIMESTAMP>, <DATE/TIME/TIMESTAMP>\\)'\\n"
- + "'\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\) OVERLAPS "
- + "\\(<DATE/TIME/TIMESTAMP>, <INTERVAL>\\)'";
+ + "'\\(<DT>, <DT>\\) OVERLAPS "
Review Comment:
The same as above.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
