[FLINK-37780][5/N] predict sql function type inference and validation #26583

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.

Sign up for GitHub

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

Jump to bottom

Merged

fsk119 merged 5 commits into apache:master from confluentinc:model-function-validation

May 28, 2025

Contributor

lihaosky commented May 22, 2025 •

edited

Loading

What is the purpose of the change

Validate predict function arguments

Brief change log

Refactor a few methods from window TVF to utils
Check first arg output is row
Check second arg is SqlModelCall
Check third arg is descriptor and columns appears in first arg output type
Check descriptor columns type can be implicitly casted to model's input type
Check Optional config arg is map of string literals
Return type inference

Verifying this change

Unit test

Does this pull request potentially affect one of the following parts:

Dependencies (does it add or upgrade a dependency): (no)
The public API, i.e., is any changed class annotated with @Public(Evolving): (no)
The serializers: (no)
The runtime per-record code paths (performance sensitive): (no)
Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
The S3 file system connector: (no)

Documentation

Does this pull request introduce a new feature? (no)
If yes, how is the feature documented? (JavaDocs)

Collaborator

flinkbot commented May 22, 2025 •

edited

Loading

CI report:

0372dbd Azure: SUCCESS

Bot commands

The @flinkbot bot supports the following commands:

@flinkbot run azure re-run the last Azure build

flinkbot added the component=<none> label

lihaosky mentioned this pull request

[FLINK-37789][3/N] introduce ml builtin sql functions and sql validator changes #26553

Merged

lihaosky changed the title ~~[FLINK-37780] predict sql function type inference and validation~~ [FLINK-37780][5/N] predict sql function type inference and validation

airlock-confluentinc bot force-pushed the model-function-validation branch 4 times, most recently from a4d988b to c5b378f Compare

May 23, 2025 18:08

fsk119 reviewed

View reviewed changes

Member

fsk119 left a comment

Thanks for your contribution. I left some comments

...-planner/src/main/java/org/apache/flink/table/planner/functions/utils/SqlValidatorUtils.java Outdated Show resolved Hide resolved

...src/main/java/org/apache/flink/table/planner/functions/sql/ml/SqlMLPredictTableFunction.java Outdated

-                          // TODO: FLINK-37780 Check operand types after integrated with SqlExplicitModelCall in
-                          // validator
-                          return false;
+                          if (!SqlValidatorUtils.checkTableAndDescriptorOperands(callBinding, 2, 1)) {

Member

fsk119 May 26, 2025

nit: Do we need to validate this again? SqlMLTableFunction#validateCall has already validated it.

Contributor Author

lihaosky May 27, 2025

validateCall doesn't check the position and count of descriptor also doesn't check first param needs to be table. validateCall can be used by both ml_predict and ml_evaluate

Contributor Author

lihaosky May 27, 2025

OK. I dropped the validation for descriptor in validateCall since it make the column name complex and failed later stage in rel converter: https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/java/org/apache/calcite/sql2rel/SqlToRelConverter.java#L2250-L2254

...src/main/java/org/apache/flink/table/planner/functions/sql/ml/SqlMLPredictTableFunction.java Show resolved Hide resolved

...src/main/java/org/apache/flink/table/planner/functions/sql/ml/SqlMLPredictTableFunction.java Outdated

		@@ -112,5 +140,92 @@ public String getAllowedSignatures(SqlOperator op, String opName) {
		return opName
		+ "(TABLE table_name, MODEL model_name, DESCRIPTOR(input_columns), [MAP[]]";

Member

fsk119 May 26, 2025

Do you miss the ) here?

...src/main/java/org/apache/flink/table/planner/functions/sql/ml/SqlMLPredictTableFunction.java Show resolved Hide resolved

...src/main/java/org/apache/flink/table/planner/functions/sql/ml/SqlMLPredictTableFunction.java

+                      return typeFactory
+                              .builder()
+                              .kind(inputRowType.getStructKind())
+                              .addAll(inputRowType.getFieldList())

Member

fsk119 May 26, 2025

Take a look at SystemOutputStrategy#inferType.

Contributor Author

lihaosky May 27, 2025

Do you mean we need to make field names unique? I'm following SqlWindowTableFunction which doesn't check if input table column has window_start etc. I'm on the fence here

Member

fsk119 May 27, 2025

Yes. Otherwise, you will get an error here.

Contributor Author

lihaosky May 27, 2025

Yes. I have a test case for this: https://github.com/apache/flink/pull/26583/files#diff-f6cf1751f150530409425ff9ada80d1cf32c44d1558b20fa6cf64664525ec998R138-R154. OK. I will allow conflict names

lihaosky added 3 commits

May 27, 2025 13:55


          [FLINK-37780] predict sql function type inference and validation

aedd365


          comments

e990770


          comments

airlock-confluentinc bot force-pushed the model-function-validation branch from 1dcf660 to 6031544 Compare

May 27, 2025 20:57


          drop descriptor validation

ab8c329

lihaosky mentioned this pull request

[FLINK-37791][6/N] model sql to rel converter #26603

Open

fix

0372dbd

fsk119 approved these changes

View reviewed changes

Member

fsk119 left a comment

LGTM

fsk119 merged commit 2cccfbf into apache:master

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

Labels

component=<none>