Mapping for Short,Byte,Date,Time,Timestamp,Boolean,BigDecimal in Java Client #1495

harshit-gangal · 2016-02-10T14:59:46Z

googlebot · 2016-02-10T14:59:47Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

enisoc · 2016-02-10T21:58:01Z

java/client/src/main/java/com/youtube/vitess/client/Proto.java

+ this.value = ByteString.copyFromUtf8(((boolean)value) ? "1" : "0");
+ } else if (value instanceof BigDecimal) {
+ // BigDecimal
+ this.type = Query.Type.FLOAT64;


I think BigDecimal should map to Query.Type.DECIMAL. Also when converting to string, I think BigDecimal.toPlainString() should work better than using setScale().

The problem with that is the precision can change.
Ex. BigDecimal(0.23), BigDecimal.toPlanString() will result in 0.2300000000000000099920072216264088638126850128173828125
In MySQL the maximum scale is 30.
So if a column is defined as my_col decimal(2,30) then this will result in wrong data in the database.

I see, so you use setScale() to limit to 30. Is that really necessary? In my tests, MySQL will do the rounding for you already; it doesn't mind if the value is longer than will fit.

The reason I think we need toPlainString() instead of toString() is because toString() will revert to exponential notation if the exponent is < -6. That is, new BigDecimal("0.0000000002").toString() will become "2E-10". That's a problem because MySQL will interpret "2E-10" as an approximate, floating point numeric literal, rather than as an exact numeric literal:

http://dev.mysql.com/doc/refman/5.6/en/precision-math-numbers.html

To make sure MySQL interprets it as an exact numeric literal, we need to send "0.0000000002", which we can get from toPlainString().

In MySQL you can have a scale of atmost 30.
So, If one gives value of BigDecimal as 0.23 and I store is as toPlainString then the value saved in database will be "0.230000000000000009992007221626" (Scale 30) and not "0.23" so on retrieval, they will get wrong value

sougou · 2016-02-10T22:36:23Z

Shouldn't there be counterpart changes to the place where results are received? For things to be symmetrical, those values need to be converted back to the native types.

enisoc · 2016-02-10T23:45:41Z

We're already converting these types on the receiving side, although we are using Joda DateTime instead of java.sql.Date and friends. We should think about whether it makes sense to switch the low-level Row class to return java.sql.Date, instead of making the JDBC wrapper convert from Joda to java.sql types.

enisoc · 2016-02-11T00:38:24Z

I've proposed #1499 to fix up the receiving side to follow standard conventions better.

enisoc · 2016-02-11T02:44:06Z

java/client/src/main/java/com/youtube/vitess/client/Proto.java

+ } else if (value instanceof Timestamp) {
+ // DateTime, TimeStamp
+ this.type = Query.Type.TIMESTAMP;
+ this.value = ByteString.copyFromUtf8(value.toString());


I found out that toString() and valueOf() on java.sql.(Date|Time|Timestamp) all use the local machine's default timezone. It'll work out as long as users only ever create java.sql.Timestamp values via valueOf(). But what if they create one from a java.util.Date or new Timestamp(millis)? They would have to adjust their epoch times to be intentionally wrong to offset for the local machine's default timezone.

Ideally we would do everything in UTC, but that means we can use neither toString() nor valueOf(). We would have to do the parsing and formatting ourselves. I'm looking into it as part of #1499 where I'm working on the receiving side.

This is known to Application Developer that is why they use SimpleDateFormat to generate date/time/timestamp field in the timezone they want and then send as setString() or setTimestamp(Timestamp.valueOf()) in the preparedStatement.

I see what you're saying. The default in JDBC when you don't specify a time zone, is to use the machine's configured time zone. Since we don't offer any way to specify time zone when sending bind variables as a map, we should follow the JDBC convention of using the machine's time zone.

FYI, in #1499 I'm adding support in Cursor for getting date/time values with a given Calendar. This should make implementing ResultSet time/date methods easier. There is also a new utility class in that PR called com.youtube.vitess.mysql.DateTime, which contains format methods that will also be useful when writing PreparedStatement setDate(..., Calendar) and so on.

I think to make that work, when implementing PreparedStatement, we'll need to send date/time bind vars to the low-level Vitess client as String, after formatting them with com.youtube.vitess.mysql.DateTime.

That can be done.

enisoc · 2016-02-17T22:17:59Z

Regarding BigDecimal, I think we're having trouble understanding each other's arguments. I've added a commit to this branch to show in code form what I'm proposing. Please see the new test case for BigDecimal that will fail if you use toString(). I've changed it to toPlainString() to show that the test passes. I kept the rounding when scale>30, but I don't think we should unnecessarily setScale(30) if the existing scale is small enough.

Lastly, regarding new BigDecimal(0.23), the "error" is not introduced by toPlainString() as you implied. The error is in using the BigDecimal(double) constructor at all. That constructor is known to produce unexpected results:

https://docs.oracle.com/javase/7/docs/api/java/math/BigDecimal.html#BigDecimal(double)

If you instead use BigDecimal.valueOf(double), the results will be as expected. I've made this change as well and added a test case to prove it works.

harshit-gangal · 2016-02-18T03:37:43Z

I understand your point now.

On Thu, Feb 18, 2016 at 3:48 AM, Anthony Yeh notifications@github.com
wrote:

Regarding BigDecimal, I think we're having trouble understanding each
other's arguments. I've added a commit to this branch to show in code form
what I'm proposing. Please see the new test case for BigDecimal that will
fail if you use toString(). I've changed it to toPlainString() to show
that the test passes. I kept the rounding when scale>30, but I don't think
we should unnecessarily setScale(30) if the existing scale is small enough.

Lastly, regarding new BigDecimal(0.23), the "error" is not introduced by
toPlainString() as you implied. The error is in using the
BigDecimal(double) constructor at all. That constructor is known to
produce unexpected results:

https://docs.oracle.com/javase/7/docs/api/java/math/BigDecimal.html#BigDecimal(double)

If you instead use BigDecimal.valueOf(double), the results will be as
expected. I've made this change as well and added a test case to prove it
works.

—
Reply to this email directly or view it on GitHub
#1495 (comment).

harshit-gangal · 2016-03-12T07:43:07Z

Review status: 0 of 2 files reviewed at latest revision, 3 unresolved discussions.

java/client/src/main/java/com/youtube/vitess/client/Proto.java, line 224 [r1] (raw file):
Done.

java/client/src/main/java/com/youtube/vitess/client/Proto.java, line 228 [r1] (raw file):
Done.

java/client/src/main/java/com/youtube/vitess/client/Proto.java, line 231 [r1] (raw file):
Done.

Comments from the review on Reviewable.io

enisoc · 2016-03-14T20:16:46Z

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

java/client/src/main/java/com/youtube/vitess/client/Proto.java, line 217 [r4] (raw file):
Is there a benefit to using String.valueOf(1) instead of just "1"?

Comments from the review on Reviewable.io

harshit-gangal · 2016-03-14T21:57:02Z

java/client/src/main/java/com/youtube/vitess/client/Proto.java, line 217 [r4] (raw file):
No Particular reason. Change Done.

Comments from the review on Reviewable.io

enisoc · 2016-03-14T22:38:43Z

Looks ready to merge, once CLA is approved.

Reviewed 1 of 1 files at r5.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks broke.

Comments from the review on Reviewable.io

googlebot · 2016-04-03T21:12:43Z

CLAs look good, thanks!

harshit-gangal · 2016-04-03T21:14:51Z

Needs to approve pull request here
https://pullapprove.com/youtube/vitess/pull-request/1495/

* feat: add failing parsing test and fix parser Signed-off-by: Manan Gupta <manan@planetscale.com> * test: add e2e tests for interval with math functions Signed-off-by: Manan Gupta <manan@planetscale.com> * test: explictly set the time-zone to prevent failures in CI Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com> Signed-off-by: Manan Gupta <manan@planetscale.com>

Mapping for Short,Byte,Date,Time,Timestamp,Boolean,BigDecimal

a8136b8

googlebot added the cla: no label Feb 10, 2016

enisoc self-assigned this Feb 10, 2016

enisoc reviewed Feb 10, 2016
View reviewed changes

enisoc reviewed Feb 11, 2016
View reviewed changes

Harshit Gangal and others added 6 commits February 16, 2016 16:16

Changes for setBoolean to INT64

2d9ea64

Merge branch 'master' into additional_bind_fields

ccd3291

Changes as per comments

aea43e8

Removing test for Date fields

e737757

Fixing Bind Variable Test

7605b47

java: Update BigDecimal bindvar test cases.

0db7ee2

Comments Incorporated

f2f0044

harshit-gangal mentioned this pull request Mar 22, 2016

JDBC Implementation #1558

Merged

Merge branch 'master' into additional_bind_fields

7216b4e

googlebot added cla: yes and removed cla: no labels Apr 3, 2016

enisoc merged commit b423c73 into master Apr 4, 2016

enisoc deleted the additional_bind_fields branch April 4, 2016 22:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mapping for Short,Byte,Date,Time,Timestamp,Boolean,BigDecimal in Java Client #1495

Mapping for Short,Byte,Date,Time,Timestamp,Boolean,BigDecimal in Java Client #1495

harshit-gangal commented Feb 10, 2016

googlebot commented Feb 10, 2016

enisoc Feb 10, 2016

harshit-gangal Feb 11, 2016

enisoc Feb 12, 2016

harshit-gangal Feb 17, 2016

sougou commented Feb 10, 2016

enisoc commented Feb 10, 2016

enisoc commented Feb 11, 2016

enisoc Feb 11, 2016

harshit-gangal Feb 11, 2016

enisoc Feb 12, 2016

harshit-gangal Feb 17, 2016

enisoc commented Feb 17, 2016

harshit-gangal commented Feb 18, 2016

harshit-gangal commented Mar 12, 2016

enisoc commented Mar 14, 2016

harshit-gangal commented Mar 14, 2016

enisoc commented Mar 14, 2016

googlebot commented Apr 3, 2016

harshit-gangal commented Apr 3, 2016

Mapping for Short,Byte,Date,Time,Timestamp,Boolean,BigDecimal in Java Client #1495

Mapping for Short,Byte,Date,Time,Timestamp,Boolean,BigDecimal in Java Client #1495

Conversation

harshit-gangal commented Feb 10, 2016

googlebot commented Feb 10, 2016

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

sougou commented Feb 10, 2016

enisoc commented Feb 10, 2016

enisoc commented Feb 11, 2016

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

enisoc commented Feb 17, 2016

harshit-gangal commented Feb 18, 2016

harshit-gangal commented Mar 12, 2016

enisoc commented Mar 14, 2016

harshit-gangal commented Mar 14, 2016

enisoc commented Mar 14, 2016

googlebot commented Apr 3, 2016

harshit-gangal commented Apr 3, 2016