[FLINK-40079][table] Reject PTF calls with sys-args if they are disabled#28675
[FLINK-40079][table] Reject PTF calls with sys-args if they are disabled#28675fhueske wants to merge 1 commit into
Conversation
Add a check in SqlValidator to reject PTF calls with system-args (on_time, uid) if the function disabled them. Generated-By: Claude Opus 4.8 (1M context)
| * <p>When a PTF sets {@code disableSystemArguments(true)}, the system arguments are not part of | ||
| * its signature. Enforcing this here covers all translation paths uniformly. | ||
| */ | ||
| private static void checkDisabledSystemArgs(SqlBasicCall call) { |
There was a problem hiding this comment.
I'm wondering why you choose this code location? Is SystemTypeInference not good enough? Changes to FlinkCalciteSqlValidator should be kept minimal.
There was a problem hiding this comment.
There are a bunch of PTF argument checks in FlinkCalciteSqlValidator already and the check at this place is fairly straightforward.
If we want to perform the check at a later point, we would need to include system args in signatures of PTFs that disable them because Calcite drops unknown named arguments when it converts them into positional arguments. So without extending the signatures, we would not know if a function was called with system args or not.
That would mean that we need to refactor quite a bit and add more special case handling for PTFs with disabled system args.
We can move the actual check implementation as a static helper into SystemTypeInference but it's much easier to call it from FlinkCalciteSqlValidator than at a later time.
What is the purpose of the change
Add a check in SqlValidator to reject PTF calls with system-args (
on_time,uid) if the function disabled them.Before, system-args were either accepted but ignored (custom implementation like MLPredict) or the generic PTF handling framework throws an exception later (this part is untouched because the execution framework expects these args in the function signature even if not set).
Brief change log
FlinkCalciteSqlValidatorProcessTableFunctionTestMLPredictfunctionVerifying this change
Added unit tests.
Does this pull request potentially affect one of the following parts:
@Public(Evolving): noDocumentation
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code 2.1.200 (Opus 4.8)