mchades commented on code in PR #9560:
URL: https://github.com/apache/gravitino/pull/9560#discussion_r2675184228
##########
core/src/main/java/org/apache/gravitino/catalog/ManagedFunctionOperations.java:
##########
@@ -131,7 +170,173 @@ public Function alterFunction(NameIdentifier ident,
FunctionChange... changes)
@Override
public boolean dropFunction(NameIdentifier ident) {
- // TODO: Implement when FunctionEntity is available
- throw new UnsupportedOperationException("dropFunction: FunctionEntity not
yet implemented");
+ try {
+ return store.delete(ident, Entity.EntityType.FUNCTION);
+ } catch (NoSuchEntityException e) {
+ return false;
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to drop function " + ident, e);
+ }
+ }
+
+ /**
+ * Converts a function identifier to a versioned identifier. The versioned
identifier uses the
+ * version number as the name to allow the store to retrieve specific
versions.
+ *
+ * @param ident The function identifier.
+ * @param version The version number, or {@link
FunctionEntity#LATEST_VERSION} for the latest.
+ * @return The versioned identifier.
+ */
+ private NameIdentifier toVersionedIdent(NameIdentifier ident, int version) {
+ return NameIdentifier.of(
+ ident.namespace().level(0),
+ ident.namespace().level(1),
+ ident.namespace().level(2),
+ ident.name(),
+ String.valueOf(version));
+ }
+
+ private Function doRegisterFunction(
+ NameIdentifier ident,
+ String comment,
+ FunctionType functionType,
+ boolean deterministic,
+ Type returnType,
+ FunctionColumn[] returnColumns,
+ FunctionDefinition[] definitions)
+ throws NoSuchSchemaException, FunctionAlreadyExistsException {
+ Preconditions.checkArgument(
+ definitions != null && definitions.length > 0,
+ "At least one function definition must be provided");
+ validateDefinitionsNoArityOverlap(definitions);
+
+ String currentUser = PrincipalUtils.getCurrentUserName();
+ Instant now = Instant.now();
+ AuditInfo auditInfo =
AuditInfo.builder().withCreator(currentUser).withCreateTime(now).build();
+
+ FunctionEntity functionEntity =
+ FunctionEntity.builder()
+ .withId(idGenerator.nextId())
+ .withName(ident.name())
+ .withNamespace(ident.namespace())
+ .withComment(comment)
+ .withFunctionType(functionType)
+ .withDeterministic(deterministic)
+ .withReturnType(returnType)
+ .withReturnColumns(returnColumns)
+ .withDefinitions(definitions)
+ .withVersion(INIT_VERSION)
+ .withAuditInfo(auditInfo)
+ .build();
+
+ try {
+ store.put(functionEntity, false /* overwrite */);
+ return functionEntity;
+
+ } catch (NoSuchEntityException e) {
+ throw new NoSuchSchemaException(e, "Schema %s does not exist",
ident.namespace());
+ } catch (EntityAlreadyExistsException e) {
+ throw new FunctionAlreadyExistsException(e, "Function %s already
exists", ident);
+ } catch (IOException e) {
+ throw new RuntimeException("Failed to register function " + ident, e);
+ }
+ }
+
+ /**
+ * Validates that all definitions in the array do not have overlapping
arities. This is used when
+ * registering a function with multiple definitions.
+ *
+ * <p>Gravitino enforces strict validation to prevent ambiguity. Operations
MUST fail if any
+ * definition's invocation arities overlap with another. For example, if an
existing definition
+ * {@code foo(int, float default 1.0)} supports arities {@code (int)} and
{@code (int, float)},
+ * adding a new definition {@code foo(int, string default 'x')} (which
supports {@code (int)} and
+ * {@code (int, string)}) will be REJECTED because both support the call
{@code foo(1)}. This
+ * ensures every function invocation deterministically maps to a single
definition.
+ *
+ * @param definitions The array of definitions to validate.
+ * @throws IllegalArgumentException If any two definitions have overlapping
arities.
+ */
+ private void validateDefinitionsNoArityOverlap(FunctionDefinition[]
definitions) {
+ for (int i = 0; i < definitions.length; i++) {
+ Set<String> aritiesI = computeArities(definitions[i]);
+ for (int j = i + 1; j < definitions.length; j++) {
+ Set<String> aritiesJ = computeArities(definitions[j]);
+ for (String arity : aritiesI) {
+ if (aritiesJ.contains(arity)) {
+ throw new IllegalArgumentException(
+ String.format(
+ "Cannot register function: definitions at index %d and %d
have overlapping "
+ + "arity '%s'. This would create ambiguous function
invocations.",
+ i, j, arity));
+ }
+ }
+ }
+ }
+ }
+
+ /**
+ * Computes all possible invocation arities for a function definition. A
definition with N
+ * parameters where the last M have default values supports arities from
(N-M) to N parameters.
+ *
+ * <p>For example:
+ *
+ * <ul>
+ * <li>{@code foo(int a)} → arities: {@code ["int"]}
+ * <li>{@code foo(int a, float b)} → arities: {@code ["int,float"]}
+ * <li>{@code foo(int a, float b default 1.0)} → arities: {@code ["int",
"int,float"]}
+ * <li>{@code foo(int a, float b default 1.0, string c default 'x')} →
arities: {@code ["int",
+ * "int,float", "int,float,string"]}
Review Comment:
No, "int,string" is not a valid arity here. Since the current arity design
is based on positional parameter matching, you cannot skip the middle parameter
b to directly pass c.
For engines that support named parameters (like Python), the parameter names
and values would be passed together at invocation time (e.g., `{a: 1, c:
"hello"}`). In that case, the engine can resolve the correct definition by
mapping parameter names to their positions and validating that any missing
parameters have default values. This would be handled at runtime by the engine,
not during function registration.
--
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]
