IGNITE-26140 Sql. Jdbc. Add new implementation for java.sql.ResultSet (adapter for org.apache.ignite.sql.ResultSet). #6507
Conversation
| */ | ||
| public class JdbcResultSet implements ResultSet { | ||
| /** Decimal format to convert string to decimal. */ | ||
| private static final ThreadLocal<DecimalFormat> decimalFormat = new ThreadLocal<>() { |
There was a problem hiding this comment.
why do we need this formatter? AFAIS, conversion from string to BD is as simple as new BigDecimal(s.trim())
|
|
||
| private int fetchSize; | ||
|
|
||
| private SqlRow currentRow; |
| return false; | ||
| } | ||
| currentRow = rs.next(); | ||
| currentPosition += 1; |
There was a problem hiding this comment.
nitpick: currentPosition++ seems more natural to me
| @Override | ||
| public boolean wasNull() throws SQLException { | ||
| ensureNotClosed(); | ||
| ensureHasCurrentRow(); |
There was a problem hiding this comment.
ensureHasCurrentRow() doesn't feel correct in this context. Assume I've read the entire dataset, and now I would like to check if the last column of the last row was null. The last column access resulted in NULL (in fact this doesn't matter, let's just stick with particular value for simplicity), but this method will throw an exception. But if I requested the very first row but but didn't access any column, this method returns false.
I think, we either need to ensure column access, or just return current value of wasNull attribute with value false in case there has not been column access yet (personally, I prefer to stick with the latter for simplicity)
|
|
||
| /** {@inheritDoc} */ | ||
| @Override | ||
| public String getString(int colIdx) throws SQLException { |
There was a problem hiding this comment.
let's mark return type of all getters as @Nullable
| @Override | ||
| public void close() throws SQLException { | ||
| closed = true; | ||
| rs.close(); |
There was a problem hiding this comment.
it's declared that method close throws java.sql.SQLException, but method close on org.apache.ignite.sql.ResultSet may throw any sort of Ignite-specific-and-not-only errors. We need to introduce exception conversion
| } else if (value instanceof Instant) { | ||
| LocalDateTime localDateTime = instantWithLocalTimeZone((Instant) value); | ||
|
|
||
| initMetadata(); |
There was a problem hiding this comment.
first, why do we need lazy initialization in case of simple wrappers like JdbcResultSetMetadata?
second, meta is used all temporal types but initialized only for value of class Instant. How is it supposed to work for LocalTime and LocalDateTime . We need to improve test coverage
ygerzhedovich
force-pushed
the
jdbc_over_thin_sql
branch
from
6d2fc42 to
d7e92ce
Compare
…et (numeric types)
…t (datetime types)
…t (datetime types)
…et (adapter for org.apache.ignite.sql.ResultSet).
Adds implementation of java.sql.ResultSet backed by ignite.sql.ResultSet.
https://issues.apache.org/jira/browse/IGNITE-26140
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
The Review Checklist
- There is a single JIRA ticket related to the pull request.
- The web-link to the pull request is attached to the JIRA ticket.
- The JIRA ticket has the Patch Available state.
- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
Notes