Copilot commented on code in PR #1378: URL: https://github.com/apache/arrow-java/pull/1378#discussion_r4326217682
########## flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalParameterCoercer.java: ########## @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.arrow.driver.jdbc.converter.impl; + +import java.math.BigDecimal; +import java.math.RoundingMode; +import org.apache.calcite.avatica.remote.TypedValue; + +/** + * Widens Avatica {@link TypedValue}s bound to DECIMAL parameters to {@link BigDecimal}, so calls + * like {@code setDouble} against an Arrow {@code Decimal} parameter bind instead of throwing. + */ +final class DecimalParameterCoercer { + + private DecimalParameterCoercer() {} + + /** + * Returns the {@link BigDecimal} value of a {@link TypedValue} bound to a DECIMAL parameter, + * scaled to what the target vector stores. + * + * <p>A value is scaled rather than rejected on a mismatch: the client sent the number it had, and + * the column's scale is what it has to fit. Rounding is HALF_UP, as {@code setBigDecimal} does. + */ + static BigDecimal toBigDecimal(TypedValue typedValue, int scale) { + return widen(typedValue).setScale(scale, RoundingMode.HALF_UP); + } + + private static BigDecimal widen(TypedValue typedValue) { + final Object local = typedValue.toLocal(); + if (local instanceof BigDecimal) { + return (BigDecimal) local; + } + 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()); + } Review Comment: `BigDecimal.valueOf(double)` throws `NumberFormatException` for NaN/Infinity. This changes failure mode compared to the previous direct cast (which would have produced a `ClassCastException` and, per the comment below, be handled by `AvaticaParameterBinder` as “unsupported binding”). To preserve consistent error handling, explicitly detect non-finite `Double`/`Float` values and fail in the same way as before (e.g., by throwing `ClassCastException` or by delegating to the final cast path). ########## flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalAvaticaParameterConverter.java: ########## @@ -31,9 +31,12 @@ public DecimalAvaticaParameterConverter(ArrowType.Decimal type) {} @Override public boolean bindParameter(FieldVector vector, TypedValue typedValue, int index) { - BigDecimal value = (BigDecimal) typedValue.toLocal(); if (vector instanceof DecimalVector) { - ((DecimalVector) vector).setSafe(index, value); + DecimalVector decimalVector = (DecimalVector) vector; + // The vector rejects any scale but its own, so widen and scale before writing. + BigDecimal value = + DecimalParameterCoercer.toBigDecimal(typedValue, decimalVector.getScale()); Review Comment: The PR description references `DecimalParameterNarrower`, but the implementation introduces/uses `DecimalParameterCoercer`. Please align the PR description with the actual class name (or rename the class) so reviewers and future maintainers can map the change rationale to the code. ########## flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalParameterCoercer.java: ########## @@ -0,0 +1,59 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.arrow.driver.jdbc.converter.impl; + +import java.math.BigDecimal; +import java.math.RoundingMode; +import org.apache.calcite.avatica.remote.TypedValue; + +/** + * Widens Avatica {@link TypedValue}s bound to DECIMAL parameters to {@link BigDecimal}, so calls + * like {@code setDouble} against an Arrow {@code Decimal} parameter bind instead of throwing. + */ +final class DecimalParameterCoercer { + + private DecimalParameterCoercer() {} + + /** + * Returns the {@link BigDecimal} value of a {@link TypedValue} bound to a DECIMAL parameter, + * scaled to what the target vector stores. + * + * <p>A value is scaled rather than rejected on a mismatch: the client sent the number it had, and + * the column's scale is what it has to fit. Rounding is HALF_UP, as {@code setBigDecimal} does. + */ + static BigDecimal toBigDecimal(TypedValue typedValue, int scale) { + return widen(typedValue).setScale(scale, RoundingMode.HALF_UP); + } + + private static BigDecimal widen(TypedValue typedValue) { + final Object local = typedValue.toLocal(); + if (local instanceof BigDecimal) { + return (BigDecimal) local; + } + 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()); + } + if (local instanceof Number) { + return BigDecimal.valueOf(((Number) local).longValue()); + } Review Comment: This branch will silently truncate/overflow for `Number` implementations that don’t fit in `long` (notably `BigInteger`), producing an incorrect decimal value. Consider handling `BigInteger` explicitly (e.g., constructing a `BigDecimal` from it) and keeping `longValue()` only for known integral primitives/wrappers where truncation cannot occur. ########## flight/flight-sql-jdbc-core/src/test/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalAvaticaParameterConverterTest.java: ########## @@ -0,0 +1,158 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.arrow.driver.jdbc.converter.impl; + +import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.math.BigDecimal; +import org.apache.arrow.driver.jdbc.utils.RootAllocatorTestExtension; +import org.apache.arrow.vector.DecimalVector; +import org.apache.arrow.vector.IntVector; +import org.apache.arrow.vector.types.pojo.ArrowType; +import org.apache.calcite.avatica.ColumnMetaData; +import org.apache.calcite.avatica.remote.TypedValue; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.RegisterExtension; + +/** + * Tests for {@link DecimalAvaticaParameterConverter}. + * + * <p>Clients call {@code setDouble} on parameters whose Arrow type is {@code Decimal}. The + * converter must widen the Avatica reps to BigDecimal and scale them to the vector instead of + * throwing. + */ +public class DecimalAvaticaParameterConverterTest { + + @RegisterExtension + public static RootAllocatorTestExtension rootAllocatorTestExtension = + new RootAllocatorTestExtension(); + + private static final int PRECISION = 38; + private static final int SCALE = 3; + + private DecimalVector vector; + private DecimalAvaticaParameterConverter converter; + + @BeforeEach + public void setUp() { + vector = + new DecimalVector( + "decimal_param", rootAllocatorTestExtension.getRootAllocator(), PRECISION, SCALE); + vector.allocateNew(5); + converter = new DecimalAvaticaParameterConverter(new ArrowType.Decimal(PRECISION, SCALE, 128)); + } + + @AfterEach + public void tearDown() { + vector.close(); + } + + @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); Review Comment: The new coercion logic introduces new edge-case behaviors for non-finite floating point values (NaN/Infinity) and for large numeric types (e.g., `BigInteger` if it can arrive via `TypedValue.toLocal()`). Adding targeted tests for these cases would help lock in the intended “unsupported binding” behavior and prevent regressions (especially given the change in exception type risk for NaN/Infinity). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
