-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[FLINK-37795] rewrite ml_evaluate to ml_predict and aggregate function #26667
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
base: master
Are you sure you want to change the base?
Conversation
de7bce9
to
b0cc113
Compare
10b36e8
to
84452ad
Compare
84452ad
to
e301fb6
Compare
} | ||
|
||
public static boolean isValidTaskType(String name) { | ||
return Arrays.stream(values()).anyMatch(taskType -> taskType.name.equals(name)); | ||
} | ||
|
||
public static Optional<RuntimeException> throwOrReturnInvalidTaskType( |
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.
I think it would make more sense to call this method validateTaskType.
String task = null; | ||
if (taskNode instanceof RexLiteral) { | ||
task = ((RexLiteral) taskNode).getValueAs(NlsString.class).getValue(); | ||
if (task == null || task.isEmpty()) { |
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.
we do not need to if (task == null
if we are going to set it to null
task = null; | ||
} | ||
} | ||
if (task == null) { |
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.
this message is not accurate as we could have a task but it not be a RexLiteral
private static String getTask(RexCall rexCall) { | ||
final RexNode taskNode = rexCall.getOperands().get(4); | ||
String task = null; | ||
if (taskNode instanceof RexLiteral) { |
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.
It would be good to include a comment as to why we need a RexLiteral here.
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.
I wonder if it would be better to have one method
String task = getRexLiteralFrom(rexCall.getOperands().get(4), true);
then all the checking and validation is done in one place.
Also I suggest removing the boolean, and always thowing an exception from the method, that the caller can catch and return as required.
if (!(scan.getCall() instanceof RexCall)) { | ||
return false; | ||
} | ||
RexCall call = (RexCall) scan.getCall(); |
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.
can't call be declared as a final also?
@@ -77,6 +76,7 @@ public Set<ConfigOption<?>> requiredOptions() { | |||
public Set<ConfigOption<?>> optionalOptions() { | |||
Set<ConfigOption<?>> options = new HashSet<>(); | |||
options.add(MODEL_VERSION); | |||
options.add(TASK); |
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.
I am curious about this test, you have added task as a config option to a test model provider factory. I notice that
OpenAIModelProviderFactory implements ModelProviderFactory
should this not pickup the task in the same way as the test here?
What is the purpose of the change
Rewrite
ml_evaluate
table function scan toml_predict
table function scan andLogicalAggreate
Brief change log
Rewrite
ml_evaluate
table function scan toml_predict
table function scan andLogicalAggreate
Verifying this change
Unit test
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes)Documentation