Repository navigation
GH-1377: [FlightRPC][Java] Bind DOUBLE to DECIMAL parameters - #1378
Open
shahriyar-novruzov wants to merge 3 commits into
Open
shahriyar-novruzov wants to merge 3 commits into
shahriyar-novruzov wants to merge 3 commits into
Conversation
DecimalAvaticaParameterConverter cast the local value to BigDecimal, which only holds when the client called setBigDecimal. TypedValue.toLocal() returns a Double for Rep.DOUBLE, so setDouble against a decimal parameter failed with "Binding value of type DOUBLE is not yet supported". The converter now widens the value and scales it to the vector, which rejects any scale but its own.
shahriyar-novruzov
requested review from
jbonofre,
laurentgo,
lidavidm and
wgtmac
as code owners
October 20, 2026 09:58
lidavidm
reviewed
Oct 20, 2026
| /** | ||
| * Converts the local value to a {@link BigDecimal}, keeping the digits the client sent. | ||
| * | ||
| * <p>{@code BigDecimal.valueOf} rather than {@code new BigDecimal(double)}: the constructor keeps |
Member
There was a problem hiding this comment.
This should be a brief code comment and not a doc comment, IMO
Author
There was a problem hiding this comment.
Moved it to a code comment on the BigDecimal.valueOf line itself. Thanks.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
3 open issues
getObject(0)without settingvalueCount. Arrow… · NewDecimalParameterNarroweris potentially misleading given the implementation and… · New
What changed in this PR
Enables JDBC-style numeric bindings (e.g., setDouble) to Arrow DECIMAL parameters by coercing Avatica TypedValue inputs into scaled BigDecimal values before writing to DecimalVector.
Changes:
- Added
DecimalParameterNarrowerto widen numeric Avatica reps intoBigDecimaland apply vector scale rounding (HALF_UP). - Updated
DecimalAvaticaParameterConverterto use the new coercion path instead of directly casting. - Added comprehensive JUnit tests for multiple numeric reps, rounding behavior, and non-decimal vectors.
| File | Description |
|---|---|
| flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalAvaticaParameterConverter.java | Uses DecimalParameterNarrower to coerce inputs to scaled BigDecimal before writing to the vector. |
| flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalParameterNarrower.java | Introduces shared logic to widen/scale Avatica values to BigDecimal with HALF_UP rounding. |
| flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalAvaticaParameterConverterTest.java | Adds coverage for DOUBLE/FLOAT/INTEGER/LONG/BigDecimal bindings, scaling/rounding, and non-decimal rejection. |
Review effort: Lite
The class widens and scales rather than narrows, so the name was misleading.
rovshan-b
approved these changes
Oct 20, 2026
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
BigDecimal.valueOf(double)throwsNumberFormatExceptionfor NaN/Infinity. This changes failure… · NewNumberimplementations that don’t fit inlong… · NewDecimalParameterNarrower, but the implementation introduces/uses… · New
3 resolved since last review
Review effort: Lite
Comment on lines
+47
to
+51
| if (local instanceof Double || local instanceof Float) { | ||
| // valueOf, not new BigDecimal(double): the constructor keeps the binary expansion, so 0.1 | ||
| // arrives as 0.1000000000000000055511151231257827. | ||
| return BigDecimal.valueOf(((Number) local).doubleValue()); | ||
| } |
Comment on lines
+52
to
+54
| if (local instanceof Number) { | ||
| return BigDecimal.valueOf(((Number) local).longValue()); | ||
| } |
Comment on lines
+68
to
+71
| @Test | ||
| public void testBindDouble() { | ||
| // The reported shape: setDouble of 123.456 against a DECIMAL(38,3) parameter. | ||
| TypedValue typedValue = TypedValue.ofLocal(ColumnMetaData.Rep.DOUBLE, 123.456d); |
Comment on lines
+37
to
+38
| BigDecimal value = | ||
| DecimalParameterCoercer.toBigDecimal(typedValue, decimalVector.getScale()); |
Copilot stopped reviewing on behalf of
lidavidm due to an error
October 20, 2026 15:46
|
Thank you for opening a pull request! Please label the PR with one or more of:
Also, add the 'breaking-change' label if appropriate. See CONTRIBUTING.md for details. |
Copilot stopped reviewing on behalf of
lidavidm due to an error
October 20, 2026 18:20
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rationale for this change
JDBC allows
setDoubleon a DECIMAL column, and tools rely on it.What changes are included in this PR?
DecimalParameterNarrowerwidens the Avatica rep toBigDecimaland scales it to the target vector; the converter calls it instead of casting.Are these changes tested?
Yes.
DecimalAvaticaParameterConverterTestcovers DOUBLE, FLOAT, INTEGER, LONG and BigDecimal binding, rounding to the vector scale, and a non-decimal vector. Reverting the converter to the cast fails 8 of its 10 tests.Are there any user-facing changes?
setDouble,setFloat,setIntandsetLongnow bind to DECIMAL parameters, andsetBigDecimalaccepts a value whose scale differs from the column's.