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

fix: trim trailing zeros in timestamp strings returned in binary mode #896

Merged
merged 1 commit into from Oct 25, 2017

Conversation

@michaelzg
Copy link
Contributor

@michaelzg michaelzg commented Jul 31, 2017

This commit trims trailing zeros from the fractional seconds in timestamp strings returned under binary mode, where the fractional seconds used to be zero padded to 6 digits. This provides consistency in the returned timestamp strings under both forced and regular binary modes.

fixes #885

@michaelzg
Copy link
Contributor Author

@michaelzg michaelzg commented Jul 31, 2017

Hm, I can look into why it failed for Postgres versions 8.3 and 8.2 ..

@codecov-io
Copy link

@codecov-io codecov-io commented Jul 31, 2017

Codecov Report

Merging #896 into master will decrease coverage by 0.02%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master     #896      +/-   ##
============================================
- Coverage     65.95%   65.92%   -0.03%     
+ Complexity     3568     3562       -6     
============================================
  Files           167      166       -1     
  Lines         15255    15242      -13     
  Branches       2469     2465       -4     
============================================
- Hits          10061    10048      -13     
+ Misses         4026     4022       -4     
- Partials       1168     1172       +4
@@ -577,30 +578,54 @@ private void timestampTestWOTZ() throws SQLException {
assertNotNull(t);
assertTrue(t.equals(TS1WOTZ));

tString = rs.getString(1);
assertNotNull(tString);
assertTrue(tString.equals(TS1WOTZ_PGFORMAT));

This comment has been minimized.

@panchenko

panchenko Aug 1, 2017

assertEquals(expected, actual) would be more readable

This comment has been minimized.

@vlsi

vlsi Aug 1, 2017
Member

+1

While the surrounding code uses assertTrue(...equals) pattern, please use assertEquals(expected, actual) so expected and actual values get printed in the failure case. See more in https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md#test

This comment has been minimized.

@michaelzg

michaelzg Aug 1, 2017
Author Contributor

Thanks, you're right it is indeed better. I will make the change in the fix commit.

I was also tempted to refactor the surrounding code in the test to instead iterate through the test cases with a test function for brevity. I was planning on doing that in a subsequent refactoring commit after tests look good--for sake of keeping the fix changes minimal.

This comment has been minimized.

@vlsi

vlsi Aug 1, 2017
Member

I was also tempted to refactor the surrounding code

Yes, that makes sense. However please use another PR if you perform that refactoring.

This comment has been minimized.

@michaelzg

michaelzg Aug 1, 2017
Author Contributor

Will do

int micros = nanos / 1000;
int microsDigits = 0;
for (int i = 0; i < 6; i++) {
if (micros % POWERS10[i + 1] != 0) {

This comment has been minimized.

@vlsi

vlsi Aug 1, 2017
Member

Frankly speaking, I would just append nanos as is, then walk backwards and remove zeros one by one.

This comment has been minimized.

@michaelzg

michaelzg Aug 1, 2017
Author Contributor

Thanks for the suggestion. Yes, that is indeed more simple.

@@ -577,30 +578,54 @@ private void timestampTestWOTZ() throws SQLException {
assertNotNull(t);
assertTrue(t.equals(TS1WOTZ));

tString = rs.getString(1);
assertNotNull(tString);
assertTrue(tString.equals(TS1WOTZ_PGFORMAT));

This comment has been minimized.

@vlsi

vlsi Aug 1, 2017
Member

+1

While the surrounding code uses assertTrue(...equals) pattern, please use assertEquals(expected, actual) so expected and actual values get printed in the failure case. See more in https://github.com/pgjdbc/pgjdbc/blob/master/CONTRIBUTING.md#test

@michaelzg
Copy link
Contributor Author

@michaelzg michaelzg commented Aug 4, 2017

Added suggested changes, but not yet had a chance to look at the ci check failures on Postgres versions 8.3 and 8.2, hopefully have some time this weekend.

I will also remind myself to squash the commits once things look ok.

int end = sb.length() - 1;
while (sb.charAt(end) == '0') {
sb.deleteCharAt(end);
end--;

This comment has been minimized.

@vlsi

vlsi Aug 4, 2017
Member

Please add a test for 00.0 kind of timestamp.
I wonder if it should get all the zeros trimmed or if one should be kept.

PS. I somehow like for(...) better

This comment has been minimized.

@michaelzg

michaelzg Aug 4, 2017
Author Contributor

hm, in that 00.0 case, it will get caught here:

if (nanos == 0) {
  return;
}

and not even the dot will be there, which conforms to a YYYY-MM-DD hh:mm:ss format for the without timezone case. I will add the test case for this though.

PS for(...) I had that earlier and after sleeping on it.. I changed it to a while loop since it read a bit better, less mental hoops to jump through.

for (int end = sb.length() - 1; sb.charAt(end) == '0'; end--) {
  sb.deleteCharAt(end);
}

I preferred the while loop after comparing the two. Thoughts?

This comment has been minimized.

@vlsi

vlsi Aug 4, 2017
Member

I will add the test case for this though.

Yes please.

I preferred the while loop after comparing the two. Thoughts?

while is fine as well.

@vlsi vlsi added this to the 42.1.5 milestone Sep 17, 2017
@vlsi vlsi added the bug label Sep 17, 2017
@vlsi
Copy link
Member

@vlsi vlsi commented Sep 17, 2017

I was about to merge this, however there are test failures at 8.2 and 8.3:

testSetTimestampWOTZ[binary = REGULAR](org.postgresql.test.jdbc2.TimestampTest)  Time elapsed: 0.061 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<...950-02-07 15:00:00.1[]> but was:<...950-02-07 15:00:00.1[0]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.postgresql.test.jdbc2.TimestampTest.timestampTestWOTZ(TimestampTest.java:608)
	at org.postgresql.test.jdbc2.TimestampTest.testSetTimestampWOTZ(TimestampTest.java:499)
testGetTimestampWOTZ[binary = REGULAR](org.postgresql.test.jdbc2.TimestampTest)  Time elapsed: 0.052 sec  <<< FAILURE!
org.junit.ComparisonFailure: expected:<...950-02-07 15:00:00.1[]> but was:<...950-02-07 15:00:00.1[0]>
	at org.junit.Assert.assertEquals(Assert.java:115)
	at org.junit.Assert.assertEquals(Assert.java:144)
	at org.postgresql.test.jdbc2.TimestampTest.timestampTestWOTZ(TimestampTest.java:608)
	at org.postgresql.test.jdbc2.TimestampTest.testGetTimestampWOTZ(TimestampTest.java:395)

It looks like 8.2 and 8.3 add zeros sometimes (the existing test fails at 8.2, 8.3 in case PR gets merged in)

@michaelpq , @davecramer , any thoughts?
Should we add some fallback logic to prevent 8.2 / 8.3 failures?

@davecramer
Copy link
Member

@davecramer davecramer commented Sep 17, 2017

Only sometimes ?

@vlsi
Copy link
Member

@vlsi vlsi commented Sep 25, 2017

@michaelzg please add "assume(server version > 8.3)" for the bad cases, so the edge case is ignored for 8.2/8.3.

@michaelzg
Copy link
Contributor Author

@michaelzg michaelzg commented Sep 26, 2017

@vlsi I've added the assumption for the edge cases, plus a little comment referring back to this issue to give context for why the assumption is there. With this, all checks seem to now have passed.

Would you like me to go ahead and squash the commits before your consideration of a merge?

@vlsi vlsi force-pushed the michaelzg:trim-trailing-zeros branch from e95815d to 0e5da05 Oct 24, 2017
This commit trims trailing zeros from the fractional seconds in timestamp strings returned under binary mode, where the fractional seconds used to be zero padded to 6 digits. This provides consistency in the returned timestamp strings under both forced and regular binary modes.

fixes #885
@vlsi vlsi force-pushed the michaelzg:trim-trailing-zeros branch from 0e5da05 to 4f22635 Oct 24, 2017
@vlsi
vlsi approved these changes Oct 24, 2017
@vlsi vlsi merged commit d28deff into pgjdbc:master Oct 25, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@vlsi vlsi modified the milestones: 42.1.5, 42.2.0 Jan 8, 2018
stellingsimon added a commit to stellingsimon/pgjdbc that referenced this pull request Feb 17, 2018
…correctly in PreparedStatement arguments

When Timestamp values with nanosecond values < 1000 are submitted as arguments to a PreparedStatement,
avoid formatting them with a trailing dot '.' after the seconds part. This fixes a regression introduced
with PR pgjdbc#896.

closes pgjdbc#1117
stellingsimon added a commit to stellingsimon/pgjdbc that referenced this pull request Feb 17, 2018
…correctly in PreparedStatement arguments

When Timestamp values with nanosecond values < 1000 are submitted as arguments to a PreparedStatement,
avoid formatting them with a trailing dot '.' after the seconds part. This fixes a regression introduced
with PR pgjdbc#896.

closes pgjdbc#1117
stellingsimon added a commit to stellingsimon/pgjdbc that referenced this pull request Feb 17, 2018
…correctly in PreparedStatement arguments

When Timestamp values with nanosecond values < 1000 are submitted as arguments to a PreparedStatement,
avoid formatting them with a trailing dot '.' after the seconds part. This fixes a regression introduced
with PR pgjdbc#896.

closes pgjdbc#1117
vlsi added a commit that referenced this pull request Feb 17, 2018
…correctly in PreparedStatement arguments (#1119)

When Timestamp values with nanosecond values < 1000 are submitted as arguments to a PreparedStatement,
avoid formatting them with a trailing dot '.' after the seconds part. This fixes a regression introduced
with PR #896.

closes #1117
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
…pgjdbc#896)

This commit trims trailing zeros from the fractional seconds in timestamp strings returned under binary mode, where the fractional seconds used to be zero padded to 6 digits. This provides consistency in the returned timestamp strings under both forced and regular binary modes.

fixes pgjdbc#885
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
…correctly in PreparedStatement arguments (pgjdbc#1119)

When Timestamp values with nanosecond values < 1000 are submitted as arguments to a PreparedStatement,
avoid formatting them with a trailing dot '.' after the seconds part. This fixes a regression introduced
with PR pgjdbc#896.

closes pgjdbc#1117
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
…pgjdbc#896)

This commit trims trailing zeros from the fractional seconds in timestamp strings returned under binary mode, where the fractional seconds used to be zero padded to 6 digits. This provides consistency in the returned timestamp strings under both forced and regular binary modes.

fixes pgjdbc#885
rhavermans added a commit to bolcom/pgjdbc that referenced this pull request Jul 13, 2018
…correctly in PreparedStatement arguments (pgjdbc#1119)

When Timestamp values with nanosecond values < 1000 are submitted as arguments to a PreparedStatement,
avoid formatting them with a trailing dot '.' after the seconds part. This fixes a regression introduced
with PR pgjdbc#896.

closes pgjdbc#1117
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

5 participants