You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "herunkang2018 (via GitHub)" <gi...@apache.org> on 2023/08/13 10:07:33 UTC
[GitHub] [calcite] herunkang2018 opened a new pull request, #3364: [CALCITE-5923] Some test cases in violates the test fixture's design principle
herunkang2018 opened a new pull request, #3364:
URL: https://github.com/apache/calcite/pull/3364
(no comment)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] herunkang2018 commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1682269453
@zabetak It seems related to the fix in [CALCITE-5678](https://issues.apache.org/jira/browse/CALCITE-5678), which change to use ISO8601 to validate the timestamp string.
I think we can safely modify this test case to use `checkFails` with message `Invalid DATE value`. And more, we may need a flag to turn this modified test case off to pass latest avatica CI pipeline, after avatica-1.24 is released, we can trun the test case on.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] herunkang2018 commented on a diff in pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1304496507
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -1253,15 +1254,28 @@ void testCastStringToDateTime(CastType castType, SqlOperatorFixture f) {
f.checkScalar("cast('1945-02-24 12:42:25.34' as TIMESTAMP(2))",
"1945-02-24 12:42:25.34", "TIMESTAMP(2) NOT NULL");
}
+ if (Bug.CALCITE_5678_FIXED) {
+ if (castType == CastType.CAST) {
+ f.checkFails("cast('1945-2-2 12:2:5' as TIMESTAMP)",
+ "Invalid DATE value, '1945-2-2 12:2:5'", true);
+ f.checkFails("cast('1241241' as TIMESTAMP)",
+ "Invalid DATE value, '1241241'", true);
+ f.checkFails("cast('1945-20-24 12:42:25.34' as TIMESTAMP)",
+ "Invalid DATE value, '1945-20-24 12:42:25.34'", true);
+ f.checkFails("cast('1945-01-24 25:42:25.34' as TIMESTAMP)",
+ "Value of HOUR field is out of range in '1945-01-24 25:42:25.34'", true);
+ f.checkFails("cast('1945-1-24 12:23:34.454' as TIMESTAMP)",
+ "Invalid DATE value, '1945-1-24 12:23:34.454'", true);
+ } else {
+ // test cases for 'SAFE_CAST' and 'TRY_CAST'
+ f.checkNull("cast('1945-2-2 12:2:5' as TIMESTAMP)");
+ f.checkNull("cast('1241241' as TIMESTAMP)");
+ f.checkNull("cast('1945-20-24 12:42:25.34' as TIMESTAMP)");
+ f.checkNull("cast('1945-01-24 25:42:25.34' as TIMESTAMP)");
+ f.checkNull("cast('1945-1-24 12:23:34.454' as TIMESTAMP)");
+ }
Review Comment:
Thanks for giving valuable comment, I will check it.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1359843557
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -1253,15 +1254,28 @@ void testCastStringToDateTime(CastType castType, SqlOperatorFixture f) {
f.checkScalar("cast('1945-02-24 12:42:25.34' as TIMESTAMP(2))",
"1945-02-24 12:42:25.34", "TIMESTAMP(2) NOT NULL");
}
+ if (Bug.CALCITE_5678_FIXED) {
+ if (castType == CastType.CAST) {
+ f.checkFails("cast('1945-2-2 12:2:5' as TIMESTAMP)",
+ "Invalid DATE value, '1945-2-2 12:2:5'", true);
+ f.checkFails("cast('1241241' as TIMESTAMP)",
+ "Invalid DATE value, '1241241'", true);
+ f.checkFails("cast('1945-20-24 12:42:25.34' as TIMESTAMP)",
+ "Invalid DATE value, '1945-20-24 12:42:25.34'", true);
+ f.checkFails("cast('1945-01-24 25:42:25.34' as TIMESTAMP)",
+ "Value of HOUR field is out of range in '1945-01-24 25:42:25.34'", true);
+ f.checkFails("cast('1945-1-24 12:23:34.454' as TIMESTAMP)",
+ "Invalid DATE value, '1945-1-24 12:23:34.454'", true);
+ } else {
+ // test cases for 'SAFE_CAST' and 'TRY_CAST'
+ f.checkNull("cast('1945-2-2 12:2:5' as TIMESTAMP)");
+ f.checkNull("cast('1241241' as TIMESTAMP)");
+ f.checkNull("cast('1945-20-24 12:42:25.34' as TIMESTAMP)");
+ f.checkNull("cast('1945-01-24 25:42:25.34' as TIMESTAMP)");
+ f.checkNull("cast('1945-1-24 12:23:34.454' as TIMESTAMP)");
+ }
Review Comment:
I think `f.checkCastFails` cannot be used to refactor the code. The reason is that`f.checkCastFails` can only check exceptions, but 'SAFE_CAST' and 'TRY_CAST' will return `NULL` instead of throwing exception if the input is not a valid value when interpreted as the desired type.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] herunkang2018 commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1679113648
The avatica error in CI seems related to CALCITE-5678, I will check it. The reason why this error appears is that it doesn't check result before, and after this fix, the test case fails.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3364: [CALCITE-5923] Some test cases in violates the test fixture's design principle
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1676314614
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1359844173
##########
core/src/main/java/org/apache/calcite/util/Bug.java:
##########
@@ -208,6 +208,12 @@ public abstract class Bug {
* MILLISECOND and MICROSECOND units in INTERVAL literal</a> is fixed. */
public static final boolean CALCITE_5422_FIXED = false;
+ /** Whether
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5678">[CALCITE-5678]
+ * Calcite should reject date literals not satisfying Gregorian calendar,
+ * per SQL standard</a> is fixed. */
+ public static final boolean CALCITE_5678_FIXED = false;
Review Comment:
Resolved.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1766928720
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [5 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [36.4% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] herunkang2018 commented on pull request #3364: [CALCITE-5923] Some test cases in violates the test fixture's design principle
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1676313410
There are test cases fails in JDK11 with avatica main, I will check the reason.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1763345626
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] herunkang2018 commented on a diff in pull request #3364: [CALCITE-5923] Some test cases in violates the test fixture's design principle
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1292744572
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -441,8 +443,8 @@ protected SqlOperatorFixture fixture() {
}
/** Generates parameters to test both regular and safe cast. */
- static Stream<Arguments> safeParameters() {
- SqlOperatorFixture f = SqlOperatorFixtureImpl.DEFAULT;
+ private Stream<Arguments> safeParameters() {
Review Comment:
We remove static here to allow use of non-static method `fixture()` to use subclass's overridden implementation, and this needs to add `@TestInstance(TestInstance.Lifecycle.PER_CLASS)` to this test class to allow this. See more at https://github.com/junit-team/junit5/issues/984
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1679161525
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1759753284
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] herunkang2018 commented on a diff in pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1304497303
##########
core/src/main/java/org/apache/calcite/util/Bug.java:
##########
@@ -208,6 +208,12 @@ public abstract class Bug {
* MILLISECOND and MICROSECOND units in INTERVAL literal</a> is fixed. */
public static final boolean CALCITE_5422_FIXED = false;
+ /** Whether
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5678">[CALCITE-5678]
+ * Calcite should reject date literals not satisfying Gregorian calendar,
+ * per SQL standard</a> is fixed. */
+ public static final boolean CALCITE_5678_FIXED = false;
Review Comment:
I will make it more concise.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1681823280
Is there a JIRA/PR for fixing the problem in Avatica? Ideally we want to merge the changes in both repos roughly at the same time otherwise the CI will remain broken till this happens and we don't want that.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1680353431
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1684657615
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] herunkang2018 commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1687270806
@zabetak I agree with you. Since it is not a bug after the changes of [CALCITE-5678](https://issues.apache.org/jira/browse/CALCITE-5678), I have changed the broken tests's in this PR and turned off them. Now the CI with avatica main is passed, and after avatica-1.24 is released, these tests can be turned on.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1362525701
##########
core/src/main/java/org/apache/calcite/util/Bug.java:
##########
@@ -208,6 +208,12 @@ public abstract class Bug {
* MILLISECOND and MICROSECOND units in INTERVAL literal</a> is fixed. */
public static final boolean CALCITE_5422_FIXED = false;
+ /** Whether
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5678">[CALCITE-5678]
+ * Calcite should reject date literals not satisfying Gregorian calendar,
+ * per SQL standard</a> is fixed. */
+ public static final boolean CALCITE_5678_FIXED = false;
Review Comment:
What do you think of https://github.com/apache/calcite/pull/3364/commits/f7caa3fb33fb33c1ef16293003ac4439dae9d571 ? Let's see if all tests run as expected.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1362949840
##########
build.gradle.kts:
##########
@@ -818,6 +818,7 @@ allprojects {
passProperty("user.language", "TR")
passProperty("user.country", "tr")
passProperty("user.timezone", "UTC")
+ passProperty("calcite.avatica.version", props.string("calcite.avatica.version"))
Review Comment:
Thanks for the code, it looks better and more reasonable.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "herunkang2018 (via GitHub)" <gi...@apache.org>.
herunkang2018 commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1362949840
##########
build.gradle.kts:
##########
@@ -818,6 +818,7 @@ allprojects {
passProperty("user.language", "TR")
passProperty("user.country", "tr")
passProperty("user.timezone", "UTC")
+ passProperty("calcite.avatica.version", props.string("calcite.avatica.version"))
Review Comment:
@zabetak Thanks for the code, it looks better and more reasonable.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle [calcite]
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak closed pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
URL: https://github.com/apache/calcite/pull/3364
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] sonarcloud[bot] commented on pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "sonarcloud[bot] (via GitHub)" <gi...@apache.org>.
sonarcloud[bot] commented on PR #3364:
URL: https://github.com/apache/calcite/pull/3364#issuecomment-1684911644
Kudos, SonarCloud Quality Gate passed! [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=3364)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=3364&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=3364&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list) [100.0% Coverage](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=3364&metric=new_duplicated_lines_density&view=list)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] zabetak commented on a diff in pull request #3364: [CALCITE-5923] Some test cases in SqlOperatorTest violates the test fixture's design principle
Posted by "zabetak (via GitHub)" <gi...@apache.org>.
zabetak commented on code in PR #3364:
URL: https://github.com/apache/calcite/pull/3364#discussion_r1304060467
##########
core/src/main/java/org/apache/calcite/util/Bug.java:
##########
@@ -208,6 +208,12 @@ public abstract class Bug {
* MILLISECOND and MICROSECOND units in INTERVAL literal</a> is fixed. */
public static final boolean CALCITE_5422_FIXED = false;
+ /** Whether
+ * <a href="https://issues.apache.org/jira/browse/CALCITE-5678">[CALCITE-5678]
+ * Calcite should reject date literals not satisfying Gregorian calendar,
+ * per SQL standard</a> is fixed. */
+ public static final boolean CALCITE_5678_FIXED = false;
Review Comment:
This is confusing since CALCITE-5678 is already fixed although not yet released. It may be better to replace this with a ticket pointing to the Avatica 1.24.0 upgrade.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -1253,15 +1254,28 @@ void testCastStringToDateTime(CastType castType, SqlOperatorFixture f) {
f.checkScalar("cast('1945-02-24 12:42:25.34' as TIMESTAMP(2))",
"1945-02-24 12:42:25.34", "TIMESTAMP(2) NOT NULL");
}
+ if (Bug.CALCITE_5678_FIXED) {
+ if (castType == CastType.CAST) {
+ f.checkFails("cast('1945-2-2 12:2:5' as TIMESTAMP)",
+ "Invalid DATE value, '1945-2-2 12:2:5'", true);
+ f.checkFails("cast('1241241' as TIMESTAMP)",
+ "Invalid DATE value, '1241241'", true);
+ f.checkFails("cast('1945-20-24 12:42:25.34' as TIMESTAMP)",
+ "Invalid DATE value, '1945-20-24 12:42:25.34'", true);
+ f.checkFails("cast('1945-01-24 25:42:25.34' as TIMESTAMP)",
+ "Value of HOUR field is out of range in '1945-01-24 25:42:25.34'", true);
+ f.checkFails("cast('1945-1-24 12:23:34.454' as TIMESTAMP)",
+ "Invalid DATE value, '1945-1-24 12:23:34.454'", true);
+ } else {
+ // test cases for 'SAFE_CAST' and 'TRY_CAST'
+ f.checkNull("cast('1945-2-2 12:2:5' as TIMESTAMP)");
+ f.checkNull("cast('1241241' as TIMESTAMP)");
+ f.checkNull("cast('1945-20-24 12:42:25.34' as TIMESTAMP)");
+ f.checkNull("cast('1945-01-24 25:42:25.34' as TIMESTAMP)");
+ f.checkNull("cast('1945-1-24 12:23:34.454' as TIMESTAMP)");
+ }
Review Comment:
Would it be possible to avoid this if-else block by using `f.checkCastFails` or something similar that takes the `castType` as parameter?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org