SPEC: Add SQL UDF spec

[flyrain]

Dev mailing thread: https://lists.apache.org/list?dev@iceberg.apache.org:lte=1M:versioned%20UDF
Design docs:

1. https://docs.google.com/document/d/1BDvOfhrH0ZQiQv9eLBqeAu8k8Vjfmeql9VzIiW1F0vc/edit?tab=t.0
2. https://docs.google.com/document/d/1E8mR-vInbQ8LDa5Lv3f22i6f8sceHojnEzxEJ6s6cvc/edit?tab=t.0#heading=h.aosih76sc44l
3. Metadata structure design doc: https://docs.google.com/document/d/1khPKL6zvWjYc5Is8HeVau6sff8FD-jNc2eLKXgit3X8/edit?tab=t.0

I have updated the metadata structure per last meeting. Here is the latest structure in a nutshell. Please use the PRs as the source of truth.

[stevenzwu]

should name be immutable? typically function signature (like Java) doesn't include parameter name

[flyrain]

The name itself doesn't have to be immutable for callers, as the order of parameter matters more. Names are mainly used by the versioned representations, they should be consistent across multiple versions. Otherwise, the rollback would be problematic. For example, we need to keep the name the same when add/rollback versions.

 { "name": "x", "type": "int", "doc": "Input integer" }
              ...
               "overload-version-id": 1,
               "deterministic": true,
               "representations": [
                  { "dialect": "trino", "body": "x + 2" }
               ],
...
               "overload-version-id": 2,
               "deterministic": true,
               "representations": [
                  { "dialect": "trino", "body": "x + 1" }
               ],

[stevenzwu]

not sure if I understand how the parameter renaming cause problem for rollback.

To change them, a new overload must be created.

Is it ok to add an overload with only parameter name change, while params type and order are the same? How would client/engine resolve to the correct overload?

[flyrain]

not sure if I understand how the parameter renaming cause problem for rollback.

Taking the example I put above. If we rename it to y at some point, then rollback to v1 or v2 will cause inconsistency between representation x+1 and parameter name y.

Is it ok to add an overload with only parameter name change, while params type and order are the same? How would client/engine resolve to the correct overload?

It shouldn't be allowed, as the signatures are the same.

[stevenzwu]

got it. basically, parameter renaming is not allowed. hence, we require the name and type are immutable.

[rdblue]

It is unclear to me what "immutable" means. Does it mean that you can't change these without updating the overload-id? That seems incorrect to me because the overload ID is more about tracking than identity. I think a better way to phrase this is:

1. Function definitions are identified by the tuple of types and there can be only one definition for a given tuple
2. All parameter names must match the definition in all versions and representations

[rdblue]

After talking with Dan about the issue we discussed in the sync, I think that it makes sense to have a list of parameter names in the SQL representation. That way each representation is self-contained and consistent. And there's no need to have restrictions on whether names can change. The names in the definition and docs are shown as the definition, but the names used in SQL are specific to that SQL. It's the same idea as having a param name in a Java interface that can differ in the definition:

interface Definition {
  int do_something(String foo);
}

class Impl implements Definition {
  int do_something(String bar) {
    return bar.length();
  }
}

[flyrain]

That sounds a good idea. To avoid duplication as most of representations may not need different names, we might still allow SQL representation to use the default parameters. So that only renaming triggers the copying of parameters to individual representations.

[flyrain]

Added an optional parameter list in the representation, also clarified that the tuple of types identify a definitioin.

[flyrain]

Per last discussion(https://lists.apache.org/thread/t30hfxydwd8qkfzk9mtscc2xpg3kf621), we keep parameters only at the definition layer.

[flyrain]

Thanks @stevenzwu for the review. Resolved all comments. Please take another look!

[rdblue]

I think this is a bit unclear because the JSON encoding produces a JSON string, like "float". This, however, is the bare float string. In addition, I don't think that we want to allow some things like decimal(9, 2) and instead want decimal(9,2). I think it's fine to refer to the JSON representation, but we probably want to use it as examples. Something like "Primitive types are encoded as simple strings, using the same representation as in ..." and "type strings must contain no spaces or quote characters".

[flyrain]

Rewrote the section per offline discussion.

[RussellSpitzer]

This is unclear to me, are we defining the file name as being the same or are we saying that a Catalog must first have a reference to the UDF and that is what must be atomically swapped?

[RussellSpitzer]

Maybe this should just be, UDF metadata files are immutable and modifications should cause a new file to be created. Catalogs can then use an atomic swap, similar to an Iceberg table, to change the UDF linked with a particular catalog identifier.

Or something?

[flyrain]

Fixed by suggestion.

[RussellSpitzer]

It what cases is this not where the UDF file is? IE, If I read this file and it's not where the location string says, is that a problem? Or is this just for future versions?

[flyrain]

Similar to the location or write.metadata.path in table and view spec, this is mainly for writer to decide where to write the metadata.json file. The reader will always get the metadata.json path from the catalogs.

[RussellSpitzer]

I'm just trying to think about a situation where you would write this file to a different place than where it already is.

[flyrain]

Not sure I understand the concern completely. I think this can happen to table and view as well, when a new metadata.json file is written to a new location controlled by write.metadata.path. The existing metadata.json files won't be moved in that cases.

[RussellSpitzer]

Probably needs more of a definition than this. Engines may not expose UDF implementation details to the end users?

[flyrain]

Here are related discussion, #14117 (comment), #14117 (comment). Are you suggesting something like this?

Engines must prevent leakage of sensitive information to end users when a function is marked as `secure` by setting the property to `true`.

[RussellSpitzer]

If i'm an engine reading this note or property, what should I do? What is "sensitive information? " what is "leakage"?

[flyrain]

I have changed it to:

1. When `secure` is set to `true`, engines must prevent leakage of sensitive information to end users. This includes
   but is not limited to: UDF definitions, error messages, logs, query plans, and intermediate results.

cc @rdblue

[RussellSpitzer]

Why do we specify it here and not in the return type? I guess it's just a hint so it doesn't really matter.

[flyrain]

Good question. Yes, this is intentionally modeled as a hint rather than part of the return type itself. The return type captures the type, while nullability is separated to provide flexibilities for engines. Different engines already treat nullability with different strictness. Spark UDF could be defined as nullable return or nonNullable return. Snowflake allow nullable return definition in certain use cases.

[RussellSpitzer]

Why is this at the definition level? Are we ok with some signatures being UDF and others being UDTF?

[flyrain]

Yes, engines(e.g., Postgres, Snowflake) usually support that.

[RussellSpitzer]

:visibly upset:

I love a function that is sometimes a scalar and sometimes not :|

[RussellSpitzer]

I'm still strongly of the opinion we should replace "definition" with "signature"

[rdblue]

Nested types?

Also, are we supporting variant? It is not considered primitive.

[flyrain]

I don't think we need to exclude variant, it doesn't seem an extra burden for UDF. Variant will work in a UDF if related engines can support it. WDYT?

[rdblue]

I think it would be more clear to say this:

Selecting the definition of a function to use is delegated to engines, which may apply their own casting rules. However, engines should:

1. Prefer exact parameter matches over safe (widening) or unsafe casts
2. Safely widen types as needed to avoid failing to find a matching definition
3. Require explicit casts for unsafe or non-obvious conversions
4. Use definitions with the same number of arguments as the input
5. Pass positional arguments in the same position as the input
6. Use definitions with the same set of field names as named input arguments

As for the last point of specifically not mixing positional and named arguments, I think that points 5 and 6 cover it. Don't reorder positional arguments and match the whole set of names. Also, implementers may ignore the "don't mix positional and named matching" but clearly stating how to match positional and named at least gives us some insurance that behavior won't be wacky if people do it anyway.

[flyrain]

Fixed per suggestion

[wgtmac]

How is the UDF metadata file referenced by table or view metadata? Does it need to be updated together with the swap? If only function-uuid is referenced, then this is not an issue.

[flyrain]

The udf name will be the identifier, just like table name, and view name. I think it's fine to go with that convention. For example, if users' sql refers a table by its identifier (ns1.t1), instead of its uuid. We may apply the similar logic there for udf.

[wgtmac]

Do we allow nullable parameter? I just saw the expected behavior if any input is null. Do we need finer-grained control?

[flyrain]

We do allow nullable parameter. The on-null-input is a hint that engines can decide whether to optimize when one of parameters is null. Please check the section "Null Input Handling" in this doc for more details, https://docs.google.com/document/d/1GC896Z4gxYP0Vz-ENqZ3tZZBqXEUQf4qDJO11NRo8F4/edit?tab=t.0

[flyrain]

Thank you all for the review. The PR is ready for another look.

[stevenzwu]

LGTM

[flyrain]

Fixed the spec related to secure and Types per today's community sync. Please take another look.