xiedeyantu commented on code in PR #4797:
URL: https://github.com/apache/calcite/pull/4797#discussion_r2828409242
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlOverlapsOperator.java:
##########
@@ -129,6 +149,14 @@ void arg(SqlWriter writer, SqlCall call, int leftPrec, int
rightPrec, int i) {
}
return false;
}
+ } else {
Review Comment:
Would it be better to have some actual SQL case descriptions? Is the above
if statement also needed?
##########
core/src/main/java/org/apache/calcite/sql/fun/SqlOverlapsOperator.java:
##########
@@ -87,7 +101,6 @@ void arg(SqlWriter writer, SqlCall call, int leftPrec, int
rightPrec, int i) {
i, i
};
- StringBuilder ret = new StringBuilder();
Review Comment:
This line doesn't need to be moved.
##########
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:
Perhaps a Javadoc is needed?
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -3307,6 +3307,75 @@ static void checkOverlaps(OverlapChecker c) {
c.isTrue("($3,$0) IMMEDIATELY SUCCEEDS ($0,$0)");
}
+ /** Test cases for <a
href="https://issues.apache.org/jira/browse/CALCITE-7418";>[CALCITE-7418]
+ * 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 "
Review Comment:
The hints here seem unclear. For example, (d1, d2) contains (d3, d4). Based
on the description, I understand that d1 can use any of the three types, and d2
can also use any of the three types. This seems to be exactly what this PR is
trying to prohibit. Is it possible to use something like: (DATETIME_TYPE,
DATETIME_TYPE) CONTAINS (DATETIME_TYPE, DATETIME_TYPE), where DATETIME_TYPE
represents one of DATE, TIME, or TIMESTAMP? I'm not sure if I truly understand
this PR.
--
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]
