[CALCITE-1703] Functions on TIMESTAMP column throws ClassCastException #971

ijokarumawak · 2018-12-13T08:44:58Z

Currently, TIMESTAMP or TIME column is used at any function, ClassCastException is thrown. For example, if a TIMESTAMP column date_added is used like this, select name, EXTRACT(YEAR FROM date_added) as year_added from FLOWFILE, Calcite generates the following code. And a ClassCastException is thrown at (Long) current[1]:

            public Object current() {
              final Object[] current = (Object[]) inputEnumerator.current();
              return new Object[] {
                  current[0],
                  (java.sql.Timestamp) current[1] == null ? (Long) null : Long.valueOf(org.apache.calcite.avatica.util.DateTimeUtils.unixDateExtract(org.apache.calcite.avatica.util.TimeUnitRange.YEAR, (Long) current[1] / 86400000L))};
            }

That happens because of the wrong if condition at PhysTypeImple.fieldReference method where it ignores the fieldType that is used as a fromType to convert the value to Long. Since fromType is null, the generated code casts current[1] to Long blindly.

The condition should accept not only java.sql.Date but also java.sql.Time and java.sql.Timestamp. Since these classes are all sub-class of java.util.Date, using isAssignableFrom method will address the issue.

Current if statement only supports java.sql.Date:

DISCLAIMER: This is my first PR to Apache Calcite project. Kindly give me some advice on unit testing.

I wanted to add an unit test to reproduce the issue and prove this fix it. But I couldn't figure out where is the right place for such test.

There is an existing case testExtractYearFromTimestamp but this test doesn't call PhysTypeImple.fieldReference. Some other test cases in the same JdbcTest such as testSumOverUnboundedPreceding does call PhysTypeImple.fieldReference. I don't know where this difference comes from..

ijokarumawak · 2018-12-14T11:07:57Z

I found that the particular code branch that can throw ClassCastException is generated when a timestamp column in an Object array row type is referenced. Referencing timestamp field at a Java class (POJO) doesn't seem having this issue.

ijokarumawak · 2018-12-19T01:32:01Z

Fixed check-style errors.

core/src/test/java/org/apache/calcite/test/ObjectArrayTableTest.java

ijokarumawak · 2018-12-26T07:36:52Z

@vlsi Thanks for reviewing! I've added more commits to address your comments. As I was working on that, I've found few more issues to fix. The added test cases illustrate what had not been working as expected.

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java

F21 · 2019-01-03T03:27:56Z

@ijokarumawak Does this supersede #405? If so, I will close that PR.

core/src/test/java/org/apache/calcite/test/ObjectArrayTableTest.java

ijokarumawak · 2019-01-07T06:16:06Z

@F21 Yes, I believe this PR covers what #405 tries to fix.

F21 · 2019-01-07T06:28:51Z

@ijokarumawak Thanks for taking this on! I'll close #405

…astException Incorporated review comments - Added the new test to CalciteSuite - Added test case message to clarify assertion objectives Adding more fixes - java.sql.Time is not converted to int correctly - java.sql.Timestamp fraction can not be used with some conditions such as '=' Fixed checkstyle errors. Better assertion messages.

ijokarumawak · 2019-01-08T01:00:41Z

Rebased with the latest master as the PR had a conflict on CalciteSuite. Also squashed commits.

ijokarumawak · 2019-01-10T10:45:59Z

@vlsi I've implemented timestamp cast and added unit test for that. The issue was the existing code has had the conversion but used scale instead of precision. Please see the added commit for what I changed. Hopefully this PR is now ready to be merged. Thanks!

risdenk · 2019-02-27T19:51:41Z

@vlsi - planning to merge still?

vlsi · 2019-02-27T19:57:16Z

core/src/test/java/org/apache/calcite/test/ObjectArrayTableTest.java

+            format(Locale.ROOT, sql, expected, actual));
+        resultSet.next();
+        assertEquals(format(Locale.ROOT, "cast(java.sql.Timestamp as %s)", castTo),
+            resultSet.getTimestamp(1), resultSet.getTimestamp(2));


@ijokarumawak , this is wrong. "EXPECTED" value must not come from a system under test.
You should either hard-code the expected value (which is preferred) or compute it with a "simple to understand" approach.

Changed to use hard-coded expected values.

vlsi · 2019-02-27T19:57:50Z

core/src/test/java/org/apache/calcite/test/ObjectArrayTableTest.java

+        final ResultSet resultSet = statement.executeQuery(
+            format(Locale.ROOT, sql, expected, actual));
+        resultSet.next();
+        assertEquals(format(Locale.ROOT, "cast(java.sql.Timestamp as %s)", castTo),


It is great you add castTo to the message, however it would be really good if you add source value as well.

Added source value.

vlsi · 2019-02-27T19:58:46Z

core/src/test/java/org/apache/calcite/test/ObjectArrayTableTest.java

+      final String sql = "select %s as EXPECTED, %s as ACTUAL from \"java_sql_date_types\"";
+
+      for (int i = 3; i >= 0; i--) {
+        final String castTo = format(Locale.ROOT, "TIMESTAMP(%d)", i);


You seem to use format(Locale.ROOT extensively. It makes the code extremely obscure. Could you just use + operator?

Alternative option is to use Kotlin's string interpolation

Changed to simple string concatenation using +.

vlsi · 2019-02-27T19:59:06Z

core/src/test/java/org/apache/calcite/test/ObjectArrayTableTest.java

+  @Test public void testCastTimestampToTimestamp() throws SQLException {
+
+    withJavaSqlDateTypes(connection -> {
+


Please avoid having to many blank lines

Removed blank lines.

ijokarumawak · 2019-04-09T02:05:10Z

Hi @vlsi, sorry for the delay. I've added another commit to address review feedback. Please let me know if there's anything more. Thanks for reviewing!

ijokarumawak · 2019-04-19T07:08:34Z

Hi @vlsi I just noticed that CI tests failed at DateRangeRulesTest. But the only thing I changed at the last commit is ObjectArrayTableTest test class. Do you know why DateRangeRulesTest failed on travis and appveyor?? The test had been successful before, and is successful if I run it locally..

michaelmior · 2019-06-05T11:55:16Z

@vlsi Could you take another look?

vlsi · 2019-06-05T12:12:51Z

@michaelmior , the review comments are resolved.

However it looks like the change fails the build, and I have not analyzed the reason.

michaelmior · 2019-06-05T12:25:03Z

Thanks!

michaelmior · 2019-06-05T12:26:49Z

@ijokarumawak What JDK version are you using? It passes on CI with JDK 10.

vlsi · 2019-06-05T12:31:47Z

@michaelmior , it does not.
I think the reason one of Travis job passes is it runs with SLOW_TESTS=Y, so only a subset of tests is executed.

ijokarumawak force-pushed the calcite-1703 branch from 80f0ee4 to c2056f6 Compare December 14, 2018 11:00

ijokarumawak force-pushed the calcite-1703 branch from c2056f6 to 4f12312 Compare December 19, 2018 01:31

vlsi requested changes Dec 24, 2018

View reviewed changes

ijokarumawak commented Dec 26, 2018

View reviewed changes

core/src/main/java/org/apache/calcite/rel/type/RelDataTypeFactoryImpl.java Show resolved Hide resolved

ijokarumawak commented Dec 26, 2018

View reviewed changes

core/src/main/java/org/apache/calcite/runtime/SqlFunctions.java Show resolved Hide resolved

vlsi requested changes Jan 3, 2019

View reviewed changes

vlsi added the returned-with-feedback There are review comments (in JIRA and/or in GitHub) to be implemented before merging the PR label Jan 4, 2019

ijokarumawak force-pushed the calcite-1703 branch from 7bd603e to db41fa5 Compare January 8, 2019 00:58

vlsi approved these changes Jan 8, 2019

View reviewed changes

vlsi added LGTM-will-merge-soon Overall PR looks OK. Only minor things left. and removed returned-with-feedback There are review comments (in JIRA and/or in GitHub) to be implemented before merging the PR labels Jan 8, 2019

Fixed time fraction cast.

ec51176

vlsi reviewed Feb 27, 2019

View reviewed changes

vlsi added returned-with-feedback There are review comments (in JIRA and/or in GitHub) to be implemented before merging the PR and removed LGTM-will-merge-soon Overall PR looks OK. Only minor things left. labels Feb 27, 2019

Incorporated review comments

c442bfd

julianhyde force-pushed the master branch from 2c19098 to f3758a6 Compare September 16, 2019 17:44

danny0405 force-pushed the master branch 2 times, most recently from 80f411d to ca27fe9 Compare November 30, 2019 07:52

vlsi force-pushed the master branch 2 times, most recently from 49cb002 to 8768a23 Compare December 29, 2019 12:07

hsyuan force-pushed the master branch from e42a79b to a0ef3c9 Compare March 29, 2020 16:53

vlsi force-pushed the master branch from e4458fa to 5462be9 Compare July 17, 2020 20:20

vlsi force-pushed the master branch from fd20efd to 3311d45 Compare December 9, 2020 20:10

julianhyde force-pushed the master branch from 52c1284 to d4e1eea Compare March 1, 2021 02:56

vlsi force-pushed the master branch from 7f65cf2 to 4bc9166 Compare March 24, 2021 09:43

zabetak force-pushed the master branch from f14cf4c to dcbc493 Compare March 10, 2022 09:13

julianhyde force-pushed the main branch from fa65a2e to 1226d1a Compare June 20, 2022 20:27

asfgit force-pushed the main branch from 9fc50f2 to e2f949d Compare September 10, 2022 16:37

asfgit force-pushed the main branch from f8f8a51 to a326bd2 Compare January 25, 2023 07:39

julianhyde force-pushed the main branch 2 times, most recently from 8a5cf83 to cf7f71b Compare June 8, 2023 21:21

tanclary force-pushed the main branch from 4804912 to 00db001 Compare September 6, 2023 00:42

libenchao force-pushed the main branch from 47db81a to 0be8eae Compare November 10, 2023 13:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CALCITE-1703] Functions on TIMESTAMP column throws ClassCastException #971

[CALCITE-1703] Functions on TIMESTAMP column throws ClassCastException #971

ijokarumawak commented Dec 13, 2018

ijokarumawak commented Dec 14, 2018

ijokarumawak commented Dec 19, 2018

ijokarumawak commented Dec 26, 2018

F21 commented Jan 3, 2019 •

edited

Loading

ijokarumawak commented Jan 7, 2019

F21 commented Jan 7, 2019

ijokarumawak commented Jan 8, 2019

ijokarumawak commented Jan 10, 2019

risdenk commented Feb 27, 2019

vlsi Feb 27, 2019

ijokarumawak Apr 9, 2019

vlsi Feb 27, 2019

ijokarumawak Apr 9, 2019

vlsi Feb 27, 2019

ijokarumawak Apr 9, 2019

vlsi Feb 27, 2019

ijokarumawak Apr 9, 2019

ijokarumawak commented Apr 9, 2019

ijokarumawak commented Apr 19, 2019

michaelmior commented Jun 5, 2019

vlsi commented Jun 5, 2019

michaelmior commented Jun 5, 2019

michaelmior commented Jun 5, 2019

vlsi commented Jun 5, 2019

		@Test public void testCastTimestampToTimestamp() throws SQLException {

		withJavaSqlDateTypes(connection -> {

[CALCITE-1703] Functions on TIMESTAMP column throws ClassCastException #971

Are you sure you want to change the base?

[CALCITE-1703] Functions on TIMESTAMP column throws ClassCastException #971

Conversation

ijokarumawak commented Dec 13, 2018

ijokarumawak commented Dec 14, 2018

ijokarumawak commented Dec 19, 2018

ijokarumawak commented Dec 26, 2018

F21 commented Jan 3, 2019 • edited Loading

ijokarumawak commented Jan 7, 2019

F21 commented Jan 7, 2019

ijokarumawak commented Jan 8, 2019

ijokarumawak commented Jan 10, 2019

risdenk commented Feb 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijokarumawak commented Apr 9, 2019

ijokarumawak commented Apr 19, 2019

michaelmior commented Jun 5, 2019

vlsi commented Jun 5, 2019

michaelmior commented Jun 5, 2019

michaelmior commented Jun 5, 2019

vlsi commented Jun 5, 2019

F21 commented Jan 3, 2019 •

edited

Loading