JDBC Implementation

[harshit-gangal]

This pull request contains changes for implementing JDBC on current Vitess client.
This is currently work in progress.

---

This change is

[googlebot]

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]

Reviewed 27 of 27 files at r1, 1 of 1 files at r2, 1 of 1 files at r4, 2 of 3 files at r5, 1 of 1 files at r6, 4 of 5 files at r7.
Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 139 [r1] (raw file):
VTGateTx commit() and rollback() calls should have .checkedGet() to make them synchronous. I'm realizing now it was dangerous to make these methods async since there's nothing to enforce that you must call checkedGet(). I'm thinking we should add blocking wrappers around VTGateConn and VTGateTx that always call checkedGet() for you, and use that instead. I'll look into that.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 157 [r1] (raw file):
Why do you catch and re-wrap SQLException, instead of letting it propagate? In the low-level API, we throw specific subclasses of SQLException to give more information. Are you hiding that information on purpose?

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 27 [r1] (raw file):
When would you not want to use bind variables? VTGate and VTTablet are both designed for use with bind variables. Vitess will be much less efficient without bind variables because nothing can be cached.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 166 [r1] (raw file):
I don't think it's right to use streamExecute() by default. Stream execute is meant for OLAP, and bypasses VTTablet's protections against slow or overly-expensive queries. I think we should use execute() by default, and possibly provide an option on VitessConnection to use streaming, which you should only do against RDONLY tablets and only from batch jobs (not serving end-user requests).

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 320 [r1] (raw file):
Is it possible to avoid filling parameterMap when USE_BIND_VARIABLES is true, and vice versa? It seems inefficient to convert every parameter to string twice (once to call setMapVariable() and once in the lower-level API when it processes bindVariables).

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 342 [r1] (raw file):
Don't you need to escape value to make sure it's safe as a quoted string literal?

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 56 [r1] (raw file):
This hides the original exception e which may contain useful information for debugging why the getFields failed. In general, I don't see why you can't just let the SQLException from getFields propagate out unchanged.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 175 [r1] (raw file):
This -1 offset is due to an unfortunate oversight on my part while implementing the Row class. I didn't notice that columnIndex is supposed to be 1-based in JDBC ResultSet. I would like to change Row to be 1-based, but first I need to make sure it won't break our existing users. I'll look into it.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 259 [r1] (raw file):
This pattern of using getString and reparsing seems inefficient. The low-level Vitess API is already parsing the raw value and returning Integer from row.getObject(). Then in this.getString() you convert that Integer back to String. Then here you take that string and parse it back to int.

If the issue is that you want to control the conversion yourself, we should instead expose a method from the low-level Vitess API to let you get the raw value without any attempt to convert. Even better would be that we should change the low-level Vitess Row class to make its getInt() behave the same way JDBC ResultSet.getInt() is supposed to. Then here you would just directly call row.getInt(). Can you comment on what might need to change to make the low-level get*() methods pass-through compatible like that?

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 668 [r1] (raw file):
Note that row.getObject() parses and converts the value every time you call it. So you are incurring the cost of parsing every time isNull() is called, and then you're incurring it again later to actually get the value.

---

Comments from the review on Reviewable.io

[enisoc]

Nice work! I think we just need to clean up some integration points between this connector and the underlying low-level Vitess API. Right now there seems to still be some mismatch in these interfaces. I mentioned some things in the Reviewable.io comments that I plan to work on, which should help with this.

[harshit-gangal]

Review status: all files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 139 [r1] (raw file):
For now. Will take care of that in jdbc classes

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 157 [r1] (raw file):
I can actually just write try and finally block. Will do that

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 27 [r1] (raw file):
We want to use bind variables every time. But, currently the client accepts very less raw data types. We know that anything can be converted to string and then send as bind variables. But, want to enhance client first and then will remove this part of the code.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 166 [r1] (raw file):
We can do that. Will update the pull request

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 56 [r1] (raw file):
Will do that

---

Comments from the review on Reviewable.io

[harshit-gangal]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 320 [r1] (raw file):
Will remove it once we merge #1495

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 342 [r1] (raw file):
This will not be required once we use bind variables only.

---

Comments from the review on Reviewable.io

[harshit-gangal]

Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks broke.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 668 [r1] (raw file):
To solve this, either we should expose getRawValue method or write a isNull method in client.

---

Comments from the review on Reviewable.io

[coveralls]

Coverage increased (+3.8%) to 60.448% when pulling f98b8af on jdbc into 81262da on master.

[coveralls]

Coverage decreased (-0.03%) to 60.45% when pulling 8304040 on jdbc into cd63133 on master.

[enisoc]

Reviewed 6 of 7 files at r16.
Review status: all files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 668 [r1] (raw file):
It should be possible to eliminate isNull() from VitessResultSet. The low-level Row class already will return null when the type allows, to indicate SQL NULL. You shouldn't need to maintain your own wasNullFlag either, since the Row class already does this and provides wasNull().

There are some places where you'll need to check if the object returned from Row is null before attempting to do type conversion. However, those will go away too once I push the type conversion down into the Row class.

---

Comments from the review on Reviewable.io

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[harshit-gangal]

Review status: 28 of 31 files reviewed at latest revision, 5 unresolved discussions.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 668 [r1] (raw file):
isNull is still needed as type conversion is done here. But have removed wasNullFlag as it was maintained in client.

---

Comments from Reviewable

[googlebot]

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.

[harshit-gangal]

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessPreparedStatement.java, line 320 [r1] (raw file):
Done.

---

Comments from Reviewable

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

[enisoc]

The CLA bot is confused by some commits from Naveen with an alternate email address, but I verified he is covered so I will override this time.

---

Reviewed 19 of 24 files at r21, 5 of 5 files at r24.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessConnection.java, line 139 [r1] (raw file):
The blocking wrappers are ready, but we can make that change after this PR so resolving this thread.

---

java/jdbc/src/main/java/com/flipkart/vitess/jdbc/VitessResultSet.java, line 668 [r1] (raw file):
Ok we will revisit whether the need for isNull() can be avoided after I push full type conversion down to the low-level client.

---

Comments from Reviewable

[enisoc]

I'm looking into the Travis failure before merging. It's the Java target that's failing.