Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CALCITE-1371: fix bug for PrepareStatement Date Parameter #279

Closed
wants to merge 2 commits into from

Conversation

yiming187
Copy link
Contributor

@yiming187
Copy link
Contributor Author

I have no idea why the CI failed. I run the test locally, 'mvn test'.

[INFO] Reactor Summary:
[INFO]
[INFO] Calcite ............................................ SUCCESS [ 1.344 s]
[INFO] Calcite Linq4j ..................................... SUCCESS [ 7.835 s]
[INFO] Calcite Core ....................................... SUCCESS [02:37 min]
[INFO] Calcite Cassandra .................................. SUCCESS [ 0.687 s]
[INFO] Calcite Druid ...................................... SUCCESS [ 0.619 s]
[INFO] Calcite Elasticsearch .............................. SUCCESS [ 0.745 s]
[INFO] Calcite Examples ................................... SUCCESS [ 0.154 s]
[INFO] Calcite Example CSV ................................ SUCCESS [ 10.027 s]
[INFO] Calcite Example Function ........................... SUCCESS [ 1.609 s]
[INFO] Calcite MongoDB .................................... SUCCESS [ 0.623 s]
[INFO] Calcite Piglet ..................................... SUCCESS [ 2.278 s]
[INFO] Calcite Plus ....................................... SUCCESS [ 1.706 s]
[INFO] Calcite Spark ...................................... SUCCESS [ 0.982 s]
[INFO] Calcite Splunk ..................................... SUCCESS [ 0.886 s]
[INFO] Calcite Ubenchmark ................................. SUCCESS [ 0.996 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 03:08 min
[INFO] Finished at: 2016-09-08T22:46:11+08:00
[INFO] Final Memory: 113M/1577M

@joshelser
Copy link
Member

It would appear that something on my environment is causing the test to fail:

Tests run: 76, Failures: 2, Errors: 0, Skipped: 4, Time elapsed: 1.588 sec <<< FAILURE! - in org.apache.calcite.avatica.RemoteDriverTest
testSignedDateParameters[0](org.apache.calcite.avatica.RemoteDriverTest)  Time elapsed: 0.008 sec  <<< FAILURE!
java.lang.AssertionError: expected:<1473336000000> but was:<1473292800000>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.junit.Assert.assertEquals(Assert.java:631)
        at org.apache.calcite.avatica.RemoteDriverTest.testSignedDateParameters(RemoteDriverTest.java:1713)

testSignedDateParameters[1](org.apache.calcite.avatica.RemoteDriverTest)  Time elapsed: 0.009 sec  <<< FAILURE!
java.lang.AssertionError: expected:<1473336000000> but was:<1473292800000>
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.failNotEquals(Assert.java:834)
        at org.junit.Assert.assertEquals(Assert.java:645)
        at org.junit.Assert.assertEquals(Assert.java:631)
        at org.apache.calcite.avatica.RemoteDriverTest.testSignedDateParameters(RemoteDriverTest.java:1713)

I'll try to figure out why since it appears that TravisCI is passing.

@julianhyde
Copy link
Contributor

julianhyde commented Sep 8, 2016

Looks like a timezone issue. You can run the test with several different timezones with e.g. mvn -Duser.timezone=Europe/Moscow test. All tests must pass in all timezones, obviously.

Can you change the commit description to something the user would understand? Users don't know about AvaticaSite.

@joshelser
Copy link
Member

So 1473336000000 == Thu Sep 08 2016 12:00:00 UTC and 1473292800000 == Thu Sep 08 2016 00:00:00 UTC (difference of 12 hours). Is this test passing for you locally, @yiming187 ? The snippet you sent earlier did not run the avatica tests. You'll want to do cd avatica; mvn package.

@joshelser
Copy link
Member

This looks good to me. I will test locally and merge.

@yiming187
Copy link
Contributor Author

Thanks, @joshelser

This is my local test result:
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary:
[INFO]
[INFO] Apache Calcite Avatica Project ..................... SUCCESS [ 1.689 s]
[INFO] Apache Calcite Avatica Metrics ..................... SUCCESS [ 1.905 s]
[INFO] Apache Calcite Avatica ............................. SUCCESS [ 16.031 s]
[INFO] Apache Calcite Avatica Dropwizard Metrics 3 ........ SUCCESS [ 0.833 s]
[INFO] Apache Calcite Avatica Noop Driver ................. SUCCESS [ 0.357 s]
[INFO] Apache Calcite Avatica Server ...................... SUCCESS [ 6.672 s]
[INFO] Apache Calcite Avatica Compatibility Kit ........... SUCCESS [ 1.652 s]
[INFO] Avatica Standalone Server .......................... SUCCESS [ 3.226 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 32.560 s
[INFO] Finished at: 2016-09-09T23:56:31+08:00
[INFO] Final Memory: 61M/1497M

@yiming187 yiming187 changed the title CALCITE-1371: fix setDate with Calendar in AvaticaSite CALCITE-1371: fix bug in PrepareStatement Date Parameter Sep 9, 2016
@yiming187 yiming187 changed the title CALCITE-1371: fix bug in PrepareStatement Date Parameter CALCITE-1371: fix bug for PrepareStatement Date Parameter Sep 9, 2016
@asfgit asfgit closed this in 23c14b5 Sep 9, 2016
@joshelser
Copy link
Member

One final thing: I did correct your commit message. Here in Calcite/Avatica, we use the form "[JIRA_ID] message", e.g. "[CALCITE-1371] fix setDate with Calendar in AvaticaSite". If you can remember to follow this next time, that would be nice.

Thanks for your contribution regardless! It is greatly appreciated.

@yiming187
Copy link
Contributor Author

Thanks a lot. I will follow the rules.

@julianhyde
Copy link
Contributor

I think you should have used the JIRA description "PreparedStatement does not process Date type correctly" for the commit description rather than "fix setDate with Calendar in AvaticaSite". The latter is what you did, but the user reading the commit log is more interested in the problem that was fixed. Also, for contributions from non-committers we append the contributor name.

@joshelser
Copy link
Member

I think you should have used the JIRA description "PreparedStatement does not process Date type correctly" for the commit description rather than "fix setDate with Calendar in AvaticaSite"

Drat, sorry, I did not think to fix that.

Also, for contributions from non-committers we append the contributor name.

My bad again. I had the git-author set to him and did not think to include his name again in the log message. I will try to remember this for the future.

Would you like me to correct these with a force-push, @julianhyde ?

@julianhyde
Copy link
Contributor

Doesn't need a force push. Not that important.

jyates pushed a commit to jyates/fineo-calcite that referenced this pull request Nov 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants