Copilot commented on code in PR #7400:
URL: https://github.com/apache/ignite-3/pull/7400#discussion_r2707754922
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/api/AsyncResultSetImpl.java:
##########
@@ -216,9 +217,13 @@ private int columnIndexChecked(String columnName) {
/** {@inheritDoc} */
@Override
public <T> T valueOrDefault(String columnName, T defaultValue) {
- T ret = (T) row.get(columnIndexChecked(columnName));
+ int columnIndex = columnIndex(columnName);
- return ret != null ? ret : defaultValue;
+ if (columnIndex == -1) {
+ return defaultValue;
+ }
+
+ return (T) row.get(columnIndex);
}
Review Comment:
The behavior of valueOrDefault has changed: previously it called
columnIndexChecked (which throws if the column doesn't exist), but now it
returns the default value when the column doesn't exist. While this appears to
be the correct behavior for a method named "valueOrDefault", this is a
potentially breaking change in behavior. The new implementation also doesn't
handle the case where the column exists but has a null value differently from
when it doesn't exist - in both cases, it returns what row.get returns.
Consider whether this behavior change is intentional and document it if so.
##########
modules/api/src/main/java/org/apache/ignite/table/TupleImpl.java:
##########
@@ -459,4 +464,122 @@ private <T> T valueNotNull(String columnName) {
return value;
}
+
+ /** Casts a {@link Number} to {@code byte}. */
+ private static byte castToByte(Object number) {
+ if (number instanceof Byte) {
+ return (byte) number;
+ }
+
+ if (number instanceof Long || number instanceof Integer || number
instanceof Short) {
+ long longVal = ((Number) number).longValue();
+ byte byteVal = ((Number) number).byteValue();
+
+ if (longVal == byteVal) {
+ return byteVal;
+ }
+
+ throw new ArithmeticException("Byte value overflow: " + number);
+ }
+
+ throw new ClassCastException(number.getClass() + " cannot be cast to "
+ byte.class);
+ }
+
+ /** Casts a {@link Number} to {@code short}. */
+ private static short castToShort(Object number) {
+ if (number instanceof Short) {
+ return (short) number;
+ }
+
+ if (number instanceof Long || number instanceof Integer || number
instanceof Byte) {
+ long longVal = ((Number) number).longValue();
+ short shortVal = ((Number) number).shortValue();
+
+ if (longVal == shortVal) {
+ return shortVal;
+ }
+
+ throw new ArithmeticException("Short value overflow: " + number);
+ }
+
+ throw new ClassCastException(number.getClass() + " cannot be cast to "
+ short.class);
+ }
+
+ /** Casts a {@link Number} to {@code int}. */
+ private static int castToInt(Object number) {
+ if (number instanceof Integer) {
+ return (int) number;
+ }
+
+ if (number instanceof Long || number instanceof Short || number
instanceof Byte) {
+ long longVal = ((Number) number).longValue();
+ int intVal = ((Number) number).intValue();
+
+ if (longVal == intVal) {
+ return intVal;
+ }
+
+ throw new ArithmeticException("Int value overflow: " + number);
+ }
+
+ throw new ClassCastException(number.getClass() + " cannot be cast to "
+ int.class);
+ }
+
+ /** Casts a {@link Number} to {@code long}. */
+ private static long castToLong(Object number) {
+ if (number instanceof Long || number instanceof Integer || number
instanceof Short || number instanceof Byte) {
+ return ((Number) number).longValue();
+ }
+
+ throw new ClassCastException(number.getClass() + " cannot be cast to "
+ long.class);
+ }
+
+ /** Casts a {@link Number} to {@code float}. */
+ private static float castToFloat(Object number) {
+ if (number instanceof Float) {
+ return (float) number;
+ }
+
+ if (number instanceof Double) {
+ double doubleVal = ((Number) number).doubleValue();
+ float floatVal = ((Number) number).floatValue();
+
+ //noinspection FloatingPointEquality
+ if (doubleVal == floatVal || Double.isNaN(doubleVal)) {
+ return floatVal;
+ }
+
+ throw new ArithmeticException("Float value overflow: " + number);
+ }
+
+ throw new ClassCastException(number.getClass() + " cannot be cast to "
+ float.class);
+ }
+
+ /** Casts a {@link Number} to {@code double}. */
+ private static double castToDouble(Object number) {
+ if (number instanceof Double || number instanceof Float) {
+ return ((Number) number).doubleValue();
+ }
+
+ throw new ClassCastException(number.getClass() + " cannot be cast to "
+ double.class);
+ }
Review Comment:
There is significant code duplication between the type casting methods in
TupleImpl (castToByte, castToShort, castToInt, castToLong, castToFloat,
castToDouble) and the corresponding methods in TupleTypeCastUtils. The
TupleImpl class is in the api module, which appears to be a public API module
that cannot depend on internal modules like core where TupleTypeCastUtils
resides. However, this duplication creates a maintenance burden where any bug
fixes or improvements to the casting logic need to be applied in two places.
Consider either refactoring the module dependencies to allow code sharing, or
moving the shared casting logic to a common module accessible from both
locations.
--
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]
