[CALCITE-1703] Functions on TIMESTAMP column throws ClassCastException

[ijokarumawak]

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]

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]

Fixed check-style errors.

[ijokarumawak]

@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.

[F21]

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

[ijokarumawak]

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

[F21]

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

[ijokarumawak]

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

[ijokarumawak]

@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]

@vlsi - planning to merge still?

[ijokarumawak]

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]

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]

@vlsi Could you take another look?

[vlsi]

@michaelmior , the review comments are resolved.

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

[michaelmior]

Thanks!

[michaelmior]

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

[vlsi]

@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.