[FLINK-38104][table] add table api support for model ml_predict #27108
Conversation
| * | ||
| * @return the context resolved model metadata. | ||
| */ | ||
| ContextResolvedModel getModel(); |
There was a problem hiding this comment.
@twalthr , this forces ContextResolvedModel to be PublicEvolving. I guess one way to avoid it is to remove this method from Model and just add it in ModelImpl
airlock-confluentinc
bot
force-pushed
the
model-table-api
branch
from
074a935 to
982f128
Compare
github-actions
bot
added
the
community-reviewed
| * runtime configuration options such as max-concurrent-operations, timeout, and execution mode | ||
| * settings. | ||
| * | ||
| * <p>Common runtime options include: |
There was a problem hiding this comment.
It's not easy for the community to maintain the javadoc about these options. How about we just link the MLPredictRuntimeConfigOptions.
| // lit() is not serializable to sql. | ||
| if (options.isEmpty()) { | ||
| return tableEnvironment.fromCall( | ||
| "ML_PREDICT", |
There was a problem hiding this comment.
use org.apache.flink.table.functions.BuiltInFunctionDefinitions#ML_PREDICT.getName() instead?
|
|
||
| @Override | ||
| public Table predict(Table table, ColumnList inputColumns, Map<String, String> options) { | ||
| // Use Expressions.map() instead of Expression.lit() to create a MAP literal since |
| table.asArgument("INPUT"), | ||
| this.asArgument("MODEL"), | ||
| Expressions.descriptor(inputColumns).asArgument("ARGS"), | ||
| Expressions.map( |
There was a problem hiding this comment.
use Expressions.mapFromArrays instead.
| .map(this::createModel) | ||
| .orElseThrow( | ||
| () -> | ||
| new ValidationException( |
|
|
||
| private final String name; | ||
| private final ContextResolvedModel model; | ||
| private final TableEnvironment env; |
|
|
||
| @Override | ||
| public <R> R accept(ExpressionVisitor<R> visitor) { | ||
| return visitor.visit(this); |
There was a problem hiding this comment.
Do we need to add a default method for ResolvedExpressionDefaultVisitor?
|
|
||
| public abstract R visit(TableReferenceExpression tableReference); | ||
|
|
||
| public abstract R visit(ModelReferenceExpression modelReferenceExpression); |
| new int[0]); | ||
| inputStack.add(relBuilder.build()); | ||
| return tableArgCall; | ||
| } else if (resolvedArg |
There was a problem hiding this comment.
I don't think it's a good idea to use instance of here. It's better we reuse ExpressionConverter to convert these expression. How about let ExpressionConverterextends ResolvedExpressionVisitor<RexNode>?
| ExecutionConfigOptions.AsyncOutputMode.ALLOW_UNORDERED) | ||
| .runTableApi( | ||
| env -> | ||
| env.fromModel("chatgpt") |
3 participants
What is the purpose of the change
Add table api support for Model and ml_predict function in https://cwiki.apache.org/confluence/display/FLINK/FLIP-526%3A+Model+ML_PREDICT%2C+ML_EVALUATE+Table+API
Brief change log
Modelinterface andModelImplimplementation for model and ml_predictfromModelPathandfromto constructModelfromTableEnvironmentModelReferenceExpressionand handle it inQueryOperationConverterContextResolvedModelVerifying this change
Unit and Integration test
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes)Documentation