-
Notifications
You must be signed in to change notification settings - Fork 25.2k
SQL: Add BigDecimal support to JDBC #56015
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
Conversation
This commit adds support for the getBigDecimal() methods.
A prepared statement will now accept a BigDecimal parameter as a proxy for a double, if the conversion is lossless.
index("library", builder -> { | ||
builder.field("name", "Don Quixote"); | ||
builder.field("page_count", 1072); | ||
}); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed an index involvement mostly due to #56013. Using an index for this test doesn't seem to bring any particular value, but can revert if I'm missing anything.
- adjust unchanged line to newly enforced line limit
Remove wrongly added chunk.
Pinging @elastic/es-ql (:Query Languages/SQL) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -547,7 +550,9 @@ public InputStream getBinaryStream(int columnIndex) throws SQLException { | |||
@Override | |||
@Deprecated | |||
public BigDecimal getBigDecimal(String columnLabel, int scale) throws SQLException { | |||
throw new SQLFeatureNotSupportedException("BigDecimal not supported"); | |||
BigDecimal bd = getBigDecimal(columnLabel); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not delegate to the other getBigDecimal
-> return getBigDecimal(column(columnLabel), scale)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, good point! Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Nice enhancements on the ResultSet testing!
Left a couple of comments.
x-pack/plugin/sql/qa/src/main/java/org/elasticsearch/xpack/sql/qa/jdbc/ResultSetTestCase.java
Outdated
Show resolved
Hide resolved
put(Long.class, Types.BIGINT); | ||
put(Float.class, Types.REAL); | ||
put(Double.class, Types.DOUBLE); | ||
// TODO: no half & scaled float testing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not adding those too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It probably should be done indeed, but I was thinking to keep it focused on BigDecimal for now, since adding coverage for half- and scaled- floats would require extending the testing for the rest of the types too.
Remove code duplicate in getBigDecimal(String columnLabel, int scale).
…/qa/jdbc/ResultSetTestCase.java Co-authored-by: Marios Trivyzas <[email protected]>
Disable the deprecation-API usage failure, since this is used to implement a deprecated API itself.
…h into enh/jdbc_bigdecimal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* SQL: Add BigDecimal support to JDBC (#56015) * Introduce BigDecimal support to JDBC -- fetching This commit adds support for the getBigDecimal() methods. * Allow BigDecimal params in double range A prepared statement will now accept a BigDecimal parameter as a proxy for a double, if the conversion is lossless. (cherry picked from commit e9a873a) * Fix compilation error Dimond notation with anonymous inner classes not avail in Java8.
This PR implements the JDBC support for BigDecimals. This is limited however, since ES support for this type is limited.
Fetching BigDecimals works with no restrictions, since all ES native and SQL numeric (and boolean) types can be accommodated by a BigDecimal.
Setting a BigDecimal value for a parameter in a prepared statement is limited to what a double can encode.
Closes #43806