Skip to content

algebra.proto argument-binding comments reference the removed Expression.Enum message and optional enum arguments #1208

Description

@nielspardon

The arguments field comments on ScalarFunction, WindowFunction and AggregateFunction still instruct implementers to bind enum arguments "followed by Enum.specified", and AggregateFunction's additionally describes optional enum arguments. Both constructs were removed from the spec some time ago, so the comments name a message that no longer exists and a feature that is no longer allowed. Comment-only, but these lines are the first thing an implementer reads when wiring up enum arguments.

Current text

proto/substrait/algebra.proto @ main (88280b1) — line 1337 (Expression.ScalarFunction.arguments = 4), line 1376 (Expression.WindowFunction.arguments = 9), line 1975 (AggregateFunction.arguments = 7):

  //  - Enum arguments must be bound using FunctionArgument.enum
  //    followed by Enum.specified, with a string that case-insensitively
  //    matches one of the allowed options.

and additionally at lines 1977-1978, on AggregateFunction only:

  //  - Optional enum arguments must be bound using FunctionArgument.enum
  //    followed by either Enum.specified or Enum.unspecified. If specified,
  //    the string must case-insensitively match one of the allowed options.

Why these are stale

Enum.specified no longer exists. FunctionArgument (line 1052) is:

message FunctionArgument {
  oneof arg_type {
    string enum = 1;
    Type type = 2;
    Expression value = 3;
  }
}

enum is a plain string — there is nothing to follow it with. The Expression.Enum message that once held oneof enum_kind { string specified = 1; Empty unspecified = 2; } was removed in f149482 ("feat(protos): remove deprecated Expression.Enum message", #1086), first released in v0.93.0, leaving only reserved 10; reserved "enum"; on Expression (line 1082). Grepping proto/ on main for message Enum or oneof enum_kind returns nothing.

Optional enum arguments no longer exist. They were removed in bd29ea3 ("feat: optional args are now specified separately from required args", #342), first released in v0.20.0, whose footer reads:

BREAKING CHANGE: optional arguments are no longer allowed to be specified as a part of FunctionArgument messages. Instead they are now specified separately as part of the function invocation.

The extension schema agrees: text/simple_extensions_schema.yaml:114-124 declares enumeration_arg with required: [options], properties name/description/options, and additionalProperties: false — a YAML cannot even spell an optional enum argument. So does the prose: site/docs/expressions/scalar_functions.md:75 gives "Required? | Yes, must be specified by the producer" and :79 "If omitted | Invalid plan", and site/docs/extensions/index.md:130 calls enumeration arguments "positional and required".

Suggested fix

Drop "followed by Enum.specified" from all three bullets, e.g.:

  //  - Enum arguments must be bound using FunctionArgument.enum, with a
  //    string that case-insensitively matches one of the allowed options.

and delete the optional-enum-argument bullet from AggregateFunction entirely.

Context

Found while reviewing a binding's implementation of enum arguments (substrait-io/substrait-python#259), where these comments were a live source of confusion about whether an unspecified enum selection was representable at all.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions