apilloud commented on a change in pull request #14464:
URL: https://github.com/apache/beam/pull/14464#discussion_r610012745
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamZetaSqlCatalog.java
##########
@@ -156,6 +159,48 @@ void addFunction(ResolvedNodes.ResolvedCreateFunctionStmt
createFunctionStmt) {
ImmutableList.of(createFunctionStmt.getSignature())));
}
+ void validateJavaUdf(ResolvedNodes.ResolvedCreateFunctionStmt
createFunctionStmt) {
+ for (FunctionArgumentType argumentType :
+ createFunctionStmt.getSignature().getFunctionArgumentList()) {
+ Type type = argumentType.getType();
+ if (type == null) {
+ throw new UnsupportedOperationException("UDF templated argument types
are not supported.");
+ }
+ validateJavaUdfZetaSqlType(type);
+ }
+ if (createFunctionStmt.getReturnType() == null) {
+ throw new NullPointerException("UDF return type must not be null.");
Review comment:
This is going to count as a crash in our SLO monitoring. Can you
actually get here or is this rejected by the parser? If you can actually get
here you might throw a `UnsupportedOperationException`, if you can't actually
get here consider `IllegalArgumentException` (which will still count as a crash
but makes it clear it isn't a problem with this code).
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamZetaSqlCatalog.java
##########
@@ -156,6 +159,48 @@ void addFunction(ResolvedNodes.ResolvedCreateFunctionStmt
createFunctionStmt) {
ImmutableList.of(createFunctionStmt.getSignature())));
}
+ void validateJavaUdf(ResolvedNodes.ResolvedCreateFunctionStmt
createFunctionStmt) {
+ for (FunctionArgumentType argumentType :
+ createFunctionStmt.getSignature().getFunctionArgumentList()) {
+ Type type = argumentType.getType();
+ if (type == null) {
+ throw new UnsupportedOperationException("UDF templated argument types
are not supported.");
+ }
+ validateJavaUdfZetaSqlType(type);
+ }
+ if (createFunctionStmt.getReturnType() == null) {
+ throw new NullPointerException("UDF return type must not be null.");
+ }
+ validateJavaUdfZetaSqlType(createFunctionStmt.getReturnType());
+ }
+
+ /**
+ * Throws {@link UnsupportedOperationException} if ZetaSQL type is not
supported in Java UDF.
+ * Supported types are a subset of the types supported by {@link
BeamJavaUdfCalcRule}.
+ */
+ void validateJavaUdfZetaSqlType(Type type) {
+ switch (type.getKind()) {
+ case TYPE_INT64:
+ case TYPE_DOUBLE:
+ case TYPE_BOOL:
+ case TYPE_STRING:
+ case TYPE_BYTES:
+ // These types are supported.
+ break;
+ case TYPE_NUMERIC:
+ case TYPE_DATE:
+ case TYPE_TIME:
+ case TYPE_DATETIME:
+ case TYPE_TIMESTAMP:
+ case TYPE_ARRAY:
+ case TYPE_STRUCT:
+ throw new UnsupportedOperationException(
+ "ZetaSQL type not allowed in Java UDF: " + type.getKind().name());
+ default:
+ throw new UnsupportedOperationException("Unknown ZetaSQL type: " +
type.getKind().name());
Review comment:
nit: possibly just make this the same exception as above? (New types are
going to be added without this list being updated, this different wording could
lead to user confusion.)
##########
File path:
sdks/java/extensions/sql/zetasql/src/main/java/org/apache/beam/sdk/extensions/sql/zetasql/BeamZetaSqlCatalog.java
##########
@@ -333,6 +380,42 @@ private void addUdfsFromSchema() {
}
}
+ private void validateScalarFunctionImpl(ScalarFunctionImpl scalarFunction) {
+ for (FunctionParameter parameter : scalarFunction.getParameters()) {
+ validateJavaUdfCalciteType(parameter.getType(typeFactory));
+ }
+ validateJavaUdfCalciteType(scalarFunction.getReturnType(typeFactory));
+ }
+
+ /**
+ * Throws {@link UnsupportedOperationException} if Calcite type is not
supported in Java UDF.
+ * Supported types are a subset of the corresponding Calcite types supported
by {@link
+ * BeamJavaUdfCalcRule}.
+ */
+ private void validateJavaUdfCalciteType(RelDataType type) {
+ switch (type.getSqlTypeName()) {
+ case BIGINT:
+ case DOUBLE:
+ case BOOLEAN:
+ case VARCHAR:
+ case VARBINARY:
+ // These types are supported.
+ break;
+ case DECIMAL:
+ case DATE:
+ case TIME:
+ case TIMESTAMP_WITH_LOCAL_TIME_ZONE:
+ case TIMESTAMP:
+ case ARRAY:
+ case ROW:
+ throw new UnsupportedOperationException(
+ "Calcite type not allowed in Java UDF: " +
type.getSqlTypeName().getName());
Review comment:
nit: Possibly make this 'ZetaSQL Java UDF'? (also possibly make this the
default exception too?)
--
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.
For queries about this service, please contact Infrastructure at:
[email protected]
