-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[FLINK-38423][table-api] Add VECTOR_SEARCH connector API #27037
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
import java.io.Serializable; | ||
|
||
/** | ||
* A {@link DynamicTableSource} that search rows of an external storage system by one or more |
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.
nits:
search ->searches
during -> at
* lazily fetch individual values from a (possibly continuously changing) external table when | ||
* necessary. | ||
* | ||
* <p>Note: Compared to {@link ScanTableSource}, a {@link VectorSearchTableSource} does only support |
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.
nits:
does only support -> only supports
I would remove the work currently.
I would remove "Further abilities are not supported." I think this is implied by the only word prior
* emitting insert-only changes currently (see also {@link RowKind}). Further abilities are not | ||
* supported. | ||
* | ||
* <p>In the last step, the planner will call {@link #getSearchRuntimeProvider(VectorSearchContext)} |
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.
Is it always the last step. It might be safer to ay after having ... then the planner will
* supported. | ||
* | ||
* <p>In the last step, the planner will call {@link #getSearchRuntimeProvider(VectorSearchContext)} | ||
* for obtaining a provider of runtime implementation. The search fields that are required to |
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.
nit:
for obtaining a provider of runtime implementation -> to obtain the runtime implementation provider
* <p>In the last step, the planner will call {@link #getSearchRuntimeProvider(VectorSearchContext)} | ||
* for obtaining a provider of runtime implementation. The search fields that are required to | ||
* perform a search are derived from a query by the planner and will be provided in the given {@link | ||
* VectorSearchTableSource.VectorSearchContext#getSearchColumns()}. The values for those key fields |
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.
nit:
those key fields -> the search fields
public interface VectorSearchTableSource extends DynamicTableSource { | ||
|
||
/** | ||
* Returns a provider of runtime implementation for reading the data. |
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.
nit:
Returns a provider of runtime implementation for reading the data. -> Returns a SearchRuntimeProvider
/** | ||
* Returns a provider of runtime implementation for reading the data. | ||
* | ||
* <p>There exist different interfaces for runtime implementation which is why {@link |
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.
nit:
VectorSearchRuntimeProvider is a base interface that should be extended (is this true) by child interfaces for specialized Vector Searches.
/** | ||
* Context for creating runtime implementation via a {@link VectorSearchRuntimeProvider}. | ||
* | ||
* <p>It offers utilities by the planner for creating runtime implementation with minimal |
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.
nit:
by the planner for -> for the planner to
* <p>It offers utilities by the planner for creating runtime implementation with minimal | ||
* dependencies to internal data structures. | ||
* | ||
* <p>Methods should be called in {@link #getSearchRuntimeProvider(VectorSearchContext)}. |
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 not sure what "Methods should be called in " means. Maybe "classes needing access to the Vector search information should use {@link #getSearchRuntimeProvider(VectorSearchContext)}"
* dependencies to internal data structures. | ||
* | ||
* <p>Methods should be called in {@link #getSearchRuntimeProvider(VectorSearchContext)}. | ||
* Returned instances that are {@link Serializable} can be directly passed into the runtime |
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 not sure what the returned instances are here, the provider or what it provides?
int[][] getSearchColumns(); | ||
|
||
/** | ||
* Runtime config provided to provider. The config can be used by planner or vector search |
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.
nits:
provider -> the provider
planner -> the planner
/** | ||
* Provides actual runtime implementation for reading the data. | ||
* | ||
* <p>There exist different interfaces for runtime implementation which is why {@link |
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.
nit:
exist -> exists
What is the purpose of the change
Add VECTOR_SEARCH connector API.
Does this pull request potentially affect one of the following parts:
@Public(Evolving)
: (yes / no)Documentation