-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-38430][table] Support runtime config for VECTOR_SEARCH #27129
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
Conversation
lihaosky
left a comment
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.
CI test failure ConfigOptionsDocsCompletenessITCase seems relevant
| <td><h5>async</h5></td> | ||
| <td style="word-wrap: break-word;">(none)</td> | ||
| <td>Boolean</td> | ||
| <td>Value can be 'true' or 'false' to suggest the planner choose the corresponding predict function. If the backend search function provider does not support the suggested mode, it will throw exception to notify users.</td> |
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.
Mention the default value?
docs/layouts/shortcodes/generated/vector_search_runtime_config_configuration.html
Outdated
Show resolved
Hide resolved
| <td><h5>output-mode</h5></td> | ||
| <td style="word-wrap: break-word;">(none)</td> | ||
| <td><p>Enum</p></td> | ||
| <td>Output mode for asynchronous operations which will convert to {@see AsyncDataStream.OutputMode}, ORDERED by default. If set to ALLOW_UNORDERED, will attempt to use {@see AsyncDataStream.OutputMode.UNORDERED} when it does not affect the correctness of the result, otherwise ORDERED will be still used.<br /><br />Possible values:<ul><li>"ORDERED"</li><li>"ALLOW_UNORDERED"</li></ul></td> |
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.
Mention default?
| public static final ConfigOption<Boolean> ASYNC = | ||
| key("async") | ||
| .booleanType() | ||
| .noDefaultValue() |
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.
Why there's no default value?
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's same as MLPredictRuntimeConfigOptions#ASYNC. I think we can align the behaviour.
| public static final ConfigOption<ExecutionConfigOptions.AsyncOutputMode> ASYNC_OUTPUT_MODE = | ||
| key("output-mode") | ||
| .enumType(ExecutionConfigOptions.AsyncOutputMode.class) | ||
| .noDefaultValue() |
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.
ditto
| public static final ConfigOption<Integer> ASYNC_MAX_CONCURRENT_OPERATIONS = | ||
| key("max-concurrent-operations") | ||
| .intType() | ||
| .noDefaultValue() |
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.
ditto. Mention default it's in TABLE_EXEC_ASYNC_VECTOR_SEARCH_MAX_CONCURRENT_OPERATIONS?
| public static final ConfigOption<Duration> ASYNC_TIMEOUT = | ||
| key("timeout") | ||
| .durationType() | ||
| .noDefaultValue() |
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.
ditto
.../main/java/org/apache/flink/table/planner/functions/sql/ml/SqlVectorSearchTableFunction.java
Outdated
Show resolved
Hide resolved
| return result; | ||
| } | ||
|
|
||
| public static Optional<RuntimeException> checkConfigValue(Map<String, String> runtimeConfig) { |
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.
checkVectorSearchConfigValue? This looks to be mainly for vector search config check
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 move it to SqlVectorSearchFunction file
|
It's not easy to fix |
lihaosky
left a comment
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.
ConfigOptionsDocsCompletenessITCase still failed with another reason. Otherwise, looks good
lihaosky
left a comment
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.
LGTM!
What is the purpose of the change
Support to using runtime config in the VECTOR_SEARCH API. For example, you can use the following statements.
Brief change log
Verifying this change
This change added tests and can be verified as follows:
Does this pull request potentially affect one of the following parts:
@Public(Evolving): (yes / no)Documentation