-
Notifications
You must be signed in to change notification settings - Fork 996
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
fix: Pgvector patch #4103
fix: Pgvector patch #4103
Conversation
Signed-off-by: cmuhao <sduxuhao@gmail.com>
Signed-off-by: cmuhao <sduxuhao@gmail.com>
"pgvector_enabled" in config.online_config | ||
and config.online_config["pgvector_enabled"] | ||
"pgvector_enabled" in config.online_store | ||
and config.online_store.pgvector_enabled |
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.
Shouldn't this be some sort of a feature view-level config? This global config makes it impossible to use postgres both for traditional feature lookup and vector search at the same time, isn't that right?
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.
That's good point. Before the pr we were unable to do that because vector is mixed with feature value. Now it's doable as vector is a new feature now. Let me see how that works
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.
@tokoko on a second thought, I would rather not mess up the feature view at the moment. Until we figure out a better design.
Also it still won't impact the original read write flow with or without the pgvrctor_enabled.
And Postgres user has to install the extension before it's able to use the pgvector, so it's better to have this config at high level as well.
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 global config makes it impossible to use postgres both for traditional feature lookup and vector search at the same time, isn't that right?
Agreed with @tokoko here. I'll approve but we should plan to tackle this soon because context aware RAG would be very useful for a case in which you want to inject structured data into the context along with the documents.
Hmmm, the get_online_feature will still work :) |
@HaoXuAI I also think this is not urgent, but we should definitely try to end up with a good design of how these two worlds will interact in the future, for example as @franciscojavierarceo pointed out, can there be a use case when a single feature service would generate the results containing results from both feature lookup and vector search? In this specific case I was referring to the fact that if |
With this pr it should work, the get_list_val_str will return null for not acceptable value type. And the vector_value field is null able. And materialization should still work in that way. |
What this PR does / why we need it:
Make the postgres online store compatible with old write and read, when the pgvector is enabled. This is done by adding a new column as
vector_value
, to store the vector value if the pgvector is enabled.Which issue(s) this PR fixes:
Fixes