gemini-code-assist[bot] commented on code in PR #38832:
URL: https://github.com/apache/beam/pull/38832#discussion_r3363510477
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/UdfImpl.java:
##########
@@ -70,17 +70,24 @@ public static Function create(Method method) {
}
/*
- * Finds a method in a given class by name.
+ * Finds a method in a given class by name. In case of overloaded methods
with the same name,
+ * this prioritizes the overload with the maximum number of parameters. This
ensures Calcite
+ * can resolve optional/default trailing parameters correctly when binding
UDF overloads.
+ *
* @param clazz class to search method in
* @param name name of the method to find
- * @return the first method with matching name or null when no method found
+ * @return the matching method with the highest parameter count or null when
no method found
*/
static @Nullable Method findMethod(Class<?> clazz, String name) {
+ Method bestMethod = null;
for (Method method : clazz.getMethods()) {
if (method.getName().equals(name) && !method.isBridge()) {
- return method;
+ if (bestMethod == null
+ || method.getParameterTypes().length >
bestMethod.getParameterTypes().length) {
+ bestMethod = method;
+ }
Review Comment:
Since `Class.getMethods()` does not guarantee any specific order, resolving
overloaded methods with the same maximum number of parameters but different
types can be non-deterministic and JVM-dependent. Introducing a deterministic
tie-breaker (such as comparing the string representation of the methods)
ensures consistent behavior across different environments.
```java
if (bestMethod == null) {
bestMethod = method;
} else {
int cmp = Integer.compare(method.getParameterTypes().length,
bestMethod.getParameterTypes().length);
if (cmp > 0 || (cmp == 0 &&
method.toString().compareTo(bestMethod.toString()) < 0)) {
bestMethod = method;
}
}
```
##########
sdks/java/extensions/sql/src/main/java/org/apache/beam/sdk/extensions/sql/impl/utils/CalciteUtils.java:
##########
@@ -396,6 +398,55 @@ public static RelDataType
sqlTypeWithAutoCast(RelDataTypeFactory typeFactory, Ty
+ ". This is currently unsupported, use List instead "
+ "of Array.");
}
+ if (type instanceof Class) {
+ Class<?> clazz = (Class<?>) type;
+ if (clazz == String.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.VARCHAR), true);
+ } else if (clazz == Integer.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.INTEGER), true);
+ } else if (clazz == int.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.INTEGER), false);
+ } else if (clazz == Long.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.BIGINT), true);
+ } else if (clazz == long.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.BIGINT), false);
+ } else if (clazz == Double.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.DOUBLE), true);
+ } else if (clazz == double.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.DOUBLE), false);
+ } else if (clazz == Float.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.FLOAT), true);
+ } else if (clazz == float.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.FLOAT), false);
+ } else if (clazz == Short.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.SMALLINT), true);
+ } else if (clazz == short.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.SMALLINT), false);
+ } else if (clazz == Byte.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.TINYINT), true);
+ } else if (clazz == byte.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.TINYINT), false);
+ } else if (clazz == Boolean.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.BOOLEAN), true);
+ } else if (clazz == boolean.class) {
+ return typeFactory.createTypeWithNullability(
+ typeFactory.createSqlType(SqlTypeName.BOOLEAN), false);
+ }
+ }
Review Comment:
The long `if-else` chain checking for each standard Java class can be
simplified and made much more maintainable by using a `switch` statement on
`clazz.getName()`. Since all primitive types are non-nullable and boxed types
are nullable, we can determine nullability dynamically using
`!clazz.isPrimitive()`, eliminating a lot of repetitive boilerplate.
```java
if (type instanceof Class) {
Class<?> clazz = (Class<?>) type;
SqlTypeName sqlTypeName = null;
switch (clazz.getName()) {
case "java.lang.String":
sqlTypeName = SqlTypeName.VARCHAR;
break;
case "java.lang.Integer":
case "int":
sqlTypeName = SqlTypeName.INTEGER;
break;
case "java.lang.Long":
case "long":
sqlTypeName = SqlTypeName.BIGINT;
break;
case "java.lang.Double":
case "double":
sqlTypeName = SqlTypeName.DOUBLE;
break;
case "java.lang.Float":
case "float":
sqlTypeName = SqlTypeName.FLOAT;
break;
case "java.lang.Short":
case "short":
sqlTypeName = SqlTypeName.SMALLINT;
break;
case "java.lang.Byte":
case "byte":
sqlTypeName = SqlTypeName.TINYINT;
break;
case "java.lang.Boolean":
case "boolean":
sqlTypeName = SqlTypeName.BOOLEAN;
break;
}
if (sqlTypeName != null) {
return typeFactory.createTypeWithNullability(
typeFactory.createSqlType(sqlTypeName), !clazz.isPrimitive());
}
}
```
--
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]
