raminqaf commented on code in PR #29844:
URL: https://github.com/apache/flink/pull/29844#discussion_r4525456257
##########
AGENTS.md:
##########
@@ -206,12 +206,50 @@ This section maps common types of Flink changes to the
modules they touch and th
### Adding a new SQL built-in function
-1. Register in `flink-table-common` in `BuiltInFunctionDefinitions.java`
(definition, input/output type strategies, runtime class reference)
-2. Implement in `flink-table-runtime` under `functions/` (extend the
appropriate base class: `BuiltInScalarFunction`, `BuiltInTableFunction`,
`BuiltInAggregateFunction`, or `BuiltInProcessTableFunction`)
-3. Add tests in `flink-table-planner` and `flink-table-runtime`
-4. Extend Table API support
-5. Document in `docs/`
-6. See
[flink-table/flink-table-planner/AGENTS.md](flink-table/flink-table-planner/AGENTS.md)
and
[flink-table/flink-table-runtime/AGENTS.md](flink-table/flink-table-runtime/AGENTS.md)
for detailed patterns
+A built-in function touches six modules. Each step names an example of that
step done right. Don't copy a whole older function: most of them use the
Calcite path, older test utilities, or test the same case at several levels.
+
+1. **Check what other engines do.** Compare the function with Spark,
Snowflake, BigQuery, DuckDB, PostgreSQL and similar engines: its name, its NULL
handling and the result type for each input type. Follow the closest one, and
state the reason for any difference in the PR.
+2. **Define it** in `BuiltInFunctionDefinitions` in `flink-table-common`:
name, kind, input and output type strategies, and how it runs, see step 3.
+ - Write the name in upper case with underscores, such as `URL_DECODE`, so
that SQL and the Table API use the same name.
+ - Reuse a strategy from `InputTypeStrategies`, `TypeStrategies`,
`SpecificInputTypeStrategies` or `SpecificTypeStrategies` before writing a new
one.
+ - When a new strategy would repeat code that other strategies share,
extract a shared abstraction instead of copying it.
`ValidationOnlyInputTypeStrategy` is one: for a function that declares its
arguments with `staticArguments(...)`, it fixes the argument count and
signature, so a subclass only implements `inferInputTypes` to validate. The
strategies of `FROM_CHANGELOG`, `TO_CHANGELOG` and `DEDUPLICATE_KEEP_FIRST` use
it.
+ - Validate literal arguments in the input strategy, so that a malformed
literal fails at planning time instead of on the first record.
+ - Register the function only here. Don't also add a field for it to
`FlinkSqlOperatorTable`: that class registers every field by reflection, and a
second operator with the same name makes compiled plans that call the function
fail to restore.
+ - Every public class in `flink-table-common` and `flink-table-api-java`,
nested classes included, needs a visibility annotation such as `@Internal`.
ArchUnit checks this in `TableApiRules`.
+3. **Choose how it runs.** The builder call in `BuiltInFunctionDefinitions`
decides:
+ - `runtimeClass("...")` is the default for scalar, table and process table
functions. The class extends `BuiltInScalarFunction`, `BuiltInTableFunction` or
`BuiltInProcessTableFunction` in `flink-table-runtime`, and the generated code
calls its `eval` method directly. Precomputing a literal argument, such as
parsing a path or compiling a pattern, is no reason for codegen. Do it once in
the constructor from `SpecializedContext`, as `VariantGetFunction` does.
+ - `runtimeProvided()` on a scalar function means that the planner generates
its code. Use it only when a runtime class can't do the job:
+ - Performance, when calls can share work. Each call of a runtime class
runs on its own, but generated code can reuse a result across the calls in one
generated method. `PARSE_JSON` and `TRY_PARSE_JSON` parse an input once for all
calls on it, which a runtime class can't do.
+ - The function reads the operator context, as `CURRENT_WATERMARK` does.
+ - The function evaluates an argument only when needed, as `COALESCE` and
`IF_NULL` do.
Review Comment:
Good catch, neither uses `runtimeProvided()`. I kept lazy evaluation as a
reason for codegen and now say that `COALESCE` and `IF_NULL` get it from a case
in `ExprCodeGenerator`, which takes precedence over the builder call. The
planner guide says the same next to the match.
##########
AGENTS.md:
##########
@@ -206,12 +206,50 @@ This section maps common types of Flink changes to the
modules they touch and th
### Adding a new SQL built-in function
-1. Register in `flink-table-common` in `BuiltInFunctionDefinitions.java`
(definition, input/output type strategies, runtime class reference)
-2. Implement in `flink-table-runtime` under `functions/` (extend the
appropriate base class: `BuiltInScalarFunction`, `BuiltInTableFunction`,
`BuiltInAggregateFunction`, or `BuiltInProcessTableFunction`)
-3. Add tests in `flink-table-planner` and `flink-table-runtime`
-4. Extend Table API support
-5. Document in `docs/`
-6. See
[flink-table/flink-table-planner/AGENTS.md](flink-table/flink-table-planner/AGENTS.md)
and
[flink-table/flink-table-runtime/AGENTS.md](flink-table/flink-table-runtime/AGENTS.md)
for detailed patterns
+A built-in function touches six modules. Each step names an example of that
step done right. Don't copy a whole older function: most of them use the
Calcite path, older test utilities, or test the same case at several levels.
+
+1. **Check what other engines do.** Compare the function with Spark,
Snowflake, BigQuery, DuckDB, PostgreSQL and similar engines: its name, its NULL
handling and the result type for each input type. Follow the closest one, and
state the reason for any difference in the PR.
+2. **Define it** in `BuiltInFunctionDefinitions` in `flink-table-common`:
name, kind, input and output type strategies, and how it runs, see step 3.
+ - Write the name in upper case with underscores, such as `URL_DECODE`, so
that SQL and the Table API use the same name.
+ - Reuse a strategy from `InputTypeStrategies`, `TypeStrategies`,
`SpecificInputTypeStrategies` or `SpecificTypeStrategies` before writing a new
one.
+ - When a new strategy would repeat code that other strategies share,
extract a shared abstraction instead of copying it.
`ValidationOnlyInputTypeStrategy` is one: for a function that declares its
arguments with `staticArguments(...)`, it fixes the argument count and
signature, so a subclass only implements `inferInputTypes` to validate. The
strategies of `FROM_CHANGELOG`, `TO_CHANGELOG` and `DEDUPLICATE_KEEP_FIRST` use
it.
+ - Validate literal arguments in the input strategy, so that a malformed
literal fails at planning time instead of on the first record.
+ - Register the function only here. Don't also add a field for it to
`FlinkSqlOperatorTable`: that class registers every field by reflection, and a
second operator with the same name makes compiled plans that call the function
fail to restore.
+ - Every public class in `flink-table-common` and `flink-table-api-java`,
nested classes included, needs a visibility annotation such as `@Internal`.
ArchUnit checks this in `TableApiRules`.
+3. **Choose how it runs.** The builder call in `BuiltInFunctionDefinitions`
decides:
+ - `runtimeClass("...")` is the default for scalar, table and process table
functions. The class extends `BuiltInScalarFunction`, `BuiltInTableFunction` or
`BuiltInProcessTableFunction` in `flink-table-runtime`, and the generated code
calls its `eval` method directly. Precomputing a literal argument, such as
parsing a path or compiling a pattern, is no reason for codegen. Do it once in
the constructor from `SpecializedContext`, as `VariantGetFunction` does.
+ - `runtimeProvided()` on a scalar function means that the planner generates
its code. Use it only when a runtime class can't do the job:
+ - Performance, when calls can share work. Each call of a runtime class
runs on its own, but generated code can reuse a result across the calls in one
generated method. `PARSE_JSON` and `TRY_PARSE_JSON` parse an input once for all
calls on it, which a runtime class can't do.
+ - The function reads the operator context, as `CURRENT_WATERMARK` does.
+ - The function evaluates an argument only when needed, as `COALESCE` and
`IF_NULL` do.
+ - `runtimeProvided()` on an aggregate function means that
`AggFunctionFactory.scala` picks the implementation, a
`BuiltInAggregateFunction` in `flink-table-runtime`. Every built-in aggregate
takes this path, because the factory can choose a class from planning facts,
such as a retracting variant when the input has updates. See `BITMAP_BUILD_AGG`.
+ - `runtimeDeferred()` maps the function to a Calcite `SqlOperator` in
`FlinkSqlOperatorTable`. Don't use it for a new function. It remains for older
functions and for syntax that the SQL parser must handle, such as
`JSON_VALUE(... RETURNING ...)`.
+4. **Implement it.** See
[flink-table/flink-table-runtime/AGENTS.md](flink-table/flink-table-runtime/AGENTS.md)
for a runtime class and
[flink-table/flink-table-planner/AGENTS.md](flink-table/flink-table-planner/AGENTS.md)
for codegen.
+ - A SQL NULL argument gives NULL, unless the function defines otherwise.
+ - A `TRY_` variant returns NULL where the function would throw. Share one
implementation, as `TryVariantGetFunction` does by passing a flag to
`VariantGetFunction`.
+ - Throw a `ValidationException` for an error found during planning, such as
in a type strategy, and a `TableRuntimeException` for an error on a record.
+ - Keep the per-record path fast, in a runtime class and in generated code
alike. `eval` and the generated call run once for every record, so a small cost
there multiplies.
+ - Do work that doesn't depend on the record once per query. A runtime
class does it in its constructor, as `VariantGetFunction` does for a literal
path. Generated code passes it in through `ctx.addReusableObject` or a reusable
member, as `StringCallGen` does for a literal pattern.
+ - Cache what depends on an argument that is not a literal but rarely
changes. `VariantGetFunction` keeps the last parsed path and parses again only
when the path changes.
Review Comment:
Right, the cache check itself converts the path. I replaced the example with
the rule: compare with a copy of the last `BinaryStringData`.
##########
AGENTS.md:
##########
@@ -196,7 +196,7 @@ Every module from the root pom.xml, organized by function.
Flink provides three
Key separations:
- **Planner vs Runtime:** The table planner generates code and execution
plans; the runtime executes them. Changes to planning logic live in
`flink-table-planner`; changes to runtime operators live in
`flink-table-runtime` or `flink-streaming-java`.
-- **Codegen vs hand-written operators:** Per-record expression logic (casts,
projections, filters, function calls) is generated at planning time by cast
rules in `flink-table-planner/.../functions/casting/` and call generators in
`flink-table-planner/.../codegen/calls/`, then compiled by Janino into the
surrounding operator class. Operators with fixed structure (joins,
aggregations, source/sink runtime) are hand-written Java in
`flink-table-runtime` or `flink-streaming-java`. New scalar functions usually
only need a `BuiltInFunctionDefinitions` entry plus a `BuiltInScalarFunction`
subclass - the planner wires up codegen automatically. New cast behaviour or a
custom call shape needs a cast rule or call generator.
+- **Codegen vs hand-written operators:** Per-record expression logic (casts,
projections, filters, function calls) is generated at planning time by cast
rules in `flink-table-planner/.../functions/casting/` and call generators in
`flink-table-planner/.../codegen/calls/`, then compiled by Janino into the
surrounding operator class. Operators with fixed structure (joins,
aggregations, source/sink runtime) are hand-written Java in
`flink-table-runtime` or `flink-streaming-java`. New scalar functions usually
only need a `BuiltInFunctionDefinitions` entry plus a `BuiltInScalarFunction`
subclass - the planner wires up codegen automatically. New cast behaviour or a
custom call shape needs a cast rule or call generator. "Adding a new SQL
built-in function" below says when a function needs codegen.
Review Comment:
Done, dropped the clause and the pointer.
##########
AGENTS.md:
##########
Review Comment:
Yes, function PRs kept hitting problems the short version didn't cover. A
missing PyFlink method fails the completeness tests in CI, and the old guide
never mentioned PyFlink. 16 methods were also missing from `expressions.rst`,
which the hotfix commit here fixes. Registering a function in both
`BuiltInFunctionDefinitions` and `FlinkSqlOperatorTable` broke compiled-plan
restore in FLINK-40255. ITCases also repeated the validation cases of the type
strategy tests. FLIP-618 adds about 25 functions, so I wanted this written down
first. To keep it maintainable, each step names one example instead of
describing the code. I can move the checklist to `flink-table/README.md` and
link it from AGENTS.md, if you prefer that for human readers.
--
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]
