-
Notifications
You must be signed in to change notification settings - Fork 192
feat: introduction of simple table functions #876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 6 commits
1635a4d
092ba9d
26fec6b
b090c4e
a4ab6e9
15f6897
96860da
b652b87
2168c5b
4a4596f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| %YAML 1.2 | ||
| --- | ||
| # Table functions: Functions that produce relations (zero or more records). | ||
| # Currently, only 0-input functions are supported - these take constant arguments | ||
| # and generate data as leaf operators. | ||
| urn: extension:io.substrait:functions_table | ||
| table_functions: | ||
| - name: "generate_series" | ||
| description: >- | ||
| Generates a series of integer values from start to stop, incrementing by step. | ||
|
|
||
| Takes constant arguments and produces zero or more records containing a single | ||
| integer value. The series includes both the start and stop values if they fall | ||
| on a step boundary. If step is positive, stops when the value exceeds stop. | ||
| If step is negative, stops when the value is less than stop. Returns empty if | ||
| step is zero or if the step direction doesn't allow reaching stop from start. | ||
| impls: | ||
| - args: | ||
| - name: start | ||
| value: i64 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i64 | ||
| description: The ending value of the series (inclusive) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like there is an interesting spilit between
I guess either way is fine... Another problem is that some systems do support generating series for other numeric types (e.g., fp32, fp64, decimal) but we can add those later.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could add this as a function option which specifies if they include or exclude the stop value. What do others think? |
||
| - name: step | ||
| value: i64 | ||
| description: The increment between values | ||
| constant: true | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://substrait.io/expressions/scalar_functions/#argument-types
My understanding is that All that being said, postgresql does in fact accept column references in the arguments to generate_series (example here), so I think it makes sense to lift this restriction.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We shouldn't unnecessarily restrict ourselves to literals and this is another place we shouldn't IMO. Implementation may not support generic expression in certain places but we shouldn't prevent it from the spec. In fact, this is another potential information to encode in dialect whether an expression has some restrictions. |
||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i64 | ||
| - args: | ||
| - name: start | ||
| value: i32 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i32 | ||
| description: The ending value of the series (inclusive) | ||
| - name: step | ||
| value: i32 | ||
| description: The increment between values | ||
| constant: true | ||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i32 | ||
| - args: | ||
| - name: start | ||
| value: i64 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i64 | ||
| description: The ending value of the series (inclusive) | ||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i64 | ||
| - args: | ||
| - name: start | ||
| value: i32 | ||
| description: The starting value of the series | ||
| - name: stop | ||
| value: i32 | ||
| description: The ending value of the series (inclusive) | ||
| return: | ||
| names: | ||
| - value | ||
| struct: | ||
| types: | ||
| - i32 | ||
| - name: "unnest" | ||
| description: Expands a list expression into a set of rows, one row per element. | ||
| impls: | ||
| - args: | ||
| - name: input | ||
| value: list<T> | ||
| description: The list to unnest | ||
| # Schema references type parameter T from list<T> | ||
| # The field type is derived from the list element type | ||
| return: | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have made this a draft PR again. I need to have a proper think about how we can represent this dynamic return type if we really want to have these functions be variadic 😰 |
||
| names: | ||
| - element | ||
| struct: | ||
| types: | ||
| - T | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -552,6 +552,61 @@ message ExpandRel { | |
| } | ||
| } | ||
|
|
||
| // Invokes a table-valued function that produces a relation (zero or more records). | ||
| // | ||
| // | ||
| // Table functions produce a table with a schema that can be: | ||
| // - Statically defined (concrete types in YAML) | ||
| // - Type-parameterized (derived from argument types, e.g., unnest(list<T>) -> {element: T}) | ||
| // - Runtime-dependent (determined by inspecting data content, use derived: false) | ||
| // | ||
| // Future extensions may add an optional input field to support transformation | ||
| // table functions that operate on input relations. | ||
| message TableFunctionRel { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is a more appropriate name for this In my mind there are two possible paths we can go down which determine the appropriate name.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From my biased view, it is better to categorize them... roughly
This gives way more modular and systematic way to describe a table function.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is your suggestion to have a different relation for each one of those? Or one shared relation? Maybe something like the following would be preferable... message TableFunctionRel {
...
oneof {
ScanLike scan_like = 1; // PR implements this one
//ProjectLike project_like = 2;
//JoinLike join_like = 3;
//GroupLike group_like = 4;
}
// implementations below
...
}
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer |
||
| RelCommon common = 1; | ||
|
|
||
| // Points to a function_anchor defined in this plan, which must refer | ||
| // to a table function in the associated YAML file. Avoid using | ||
| // anchor/reference zero. | ||
| uint32 function_reference = 2; | ||
|
|
||
| // The arguments to be bound to the function. This must have exactly the | ||
| // number of arguments specified in the function definition from the YAML file, | ||
| // and the argument types must also match exactly: | ||
| // - Value arguments must be bound using FunctionArgument.value. | ||
| // Currently (0-input functions only), expressions must be constants | ||
|
benbellick marked this conversation as resolved.
Outdated
|
||
| // (literals or expressions evaluable without input data). | ||
| // - Type arguments must be bound using FunctionArgument.type. | ||
| // - Enum arguments must be bound using FunctionArgument.enum with a | ||
| // string that case-insensitively matches one of the allowed options. | ||
| repeated FunctionArgument arguments = 3; | ||
|
|
||
| // Options to specify behavior for corner cases, or leave behavior | ||
| // unspecified if the consumer does not need specific behavior in these | ||
| // cases. | ||
| repeated FunctionOption options = 5; | ||
|
|
||
| // Indicates whether the schema was derived from the YAML function definition: | ||
| // - If true: the table_schema was computed from the ExpressionNamedStruct in the | ||
| // YAML file by evaluating it with the bound argument types. This includes: | ||
| // * Static schemas (concrete field types) | ||
| // * Type-parameterized schemas (e.g., T from list<T>) | ||
| // - If false: the table_schema was determined by the plan producer based on runtime | ||
| // data inspection (YAML omits the return field) | ||
|
benbellick marked this conversation as resolved.
Outdated
|
||
| // | ||
| // This value must be true if and only if a return field is provided in the YAML | ||
| // definition of this function. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we simply have
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wanted to be consistent with the behavior of message ScalarFunction {
...
// Must be set to the return type of the function, exactly as derived
// using the declaration in the extension.
Type output_type = 3;
...
}I figured that it makes the plans more explicit to always include the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is fine to have |
||
| bool derived = 6; | ||
|
|
||
| // The concrete output schema of the relation. For derived schemas (derived=true), | ||
| // this must match the schema computed by evaluating the YAML ExpressionNamedStruct | ||
| // with the bound argument types. For runtime-dependent schemas (derived=false), | ||
| // this is provided by the plan producer. | ||
| NamedStruct table_schema = 7; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am wondering whether we can omit
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm okay with omitting
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving this conversation unresolved if others have something to say, but my most recent commit has dropped |
||
|
|
||
| substrait.extensions.AdvancedExtension advanced_extension = 8; | ||
| } | ||
|
|
||
| // A relation with output field names. | ||
| // | ||
| // This is for use at the root of a `Rel` tree. | ||
|
|
@@ -581,6 +636,7 @@ message Rel { | |
| WriteRel write = 19; | ||
| DdlRel ddl = 20; | ||
| UpdateRel update = 22; | ||
| TableFunctionRel table_function = 23; | ||
| // Physical relations | ||
| HashJoinRel hash_join = 13; | ||
| MergeJoinRel merge_join = 14; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to say something about the default value of
step?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to instead drop the implementations where
stepis missing? That way, we force the plans to be more explicit.