Copilot commented on code in PR #1378: URL: https://github.com/apache/arrow-java/pull/1378#discussion_r4324341699
########## flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalParameterNarrower.java: ########## @@ -0,0 +1,62 @@ +/* + * 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 DecimalParameterNarrower { + + private DecimalParameterNarrower() {} + + /** + * 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); + } + + /** + * 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 + * the binary expansion, so 0.1 arrives as 0.1000000000000000055511151231257827. + */ + 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) { + return BigDecimal.valueOf(((Number) local).doubleValue()); + } + if (local instanceof Number) { + return BigDecimal.valueOf(((Number) local).longValue()); + } + // Anything else keeps the upstream failure: a cast error the binder reports as unsupported. + return (BigDecimal) local; Review Comment: Using a forced cast as control flow for unsupported types makes failures harder to diagnose (the resulting `ClassCastException` won’t include intent/context like rep/type). Prefer an explicit, intentional failure mode here (e.g., throwing an `IllegalArgumentException` with details about `typedValue.type`/`local.getClass()`), or returning a sentinel that the caller can use to return `false` for unsupported bindings. ########## 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); + + boolean result = converter.bindParameter(vector, typedValue, 0); + + assertTrue(result); + assertThat(vector.getObject(0), is(new BigDecimal("123.456"))); Review Comment: These tests read from an Arrow vector via `getObject(0)` without setting `valueCount`. Arrow vectors commonly require `setValueCount(...)` to define the readable range, and relying on implicit behavior can make tests brittle across Arrow versions/implementations. Consider setting `vector.setValueCount(1)` (or matching the allocated size) before asserting on `getObject(...)`, or updating the test setup to establish a value count consistently. ########## flight/flight-sql-jdbc-core/src/main/java/org/apache/arrow/driver/jdbc/converter/impl/DecimalParameterNarrower.java: ########## @@ -0,0 +1,62 @@ +/* + * 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 DecimalParameterNarrower { Review Comment: The class name `DecimalParameterNarrower` is potentially misleading given the implementation and Javadoc emphasize *widening* values to `BigDecimal` and then scaling/rounding. If there isn’t an existing project convention driving this name, consider renaming to something that reflects coercion/scaling (e.g., `DecimalParameterCoercer` or `DecimalParameterScaler`) to reduce confusion for future maintainers. -- 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]
