-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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 storeindex cosmosnosql query issue - (BadRequest) One of the inpu… #17385
base: main
Are you sure you want to change the base?
fix storeindex cosmosnosql query issue - (BadRequest) One of the inpu… #17385
Conversation
…t values is invalid.
@@ -325,25 +325,31 @@ def _query(self, query: VectorStoreQuery, **kwargs: Any) -> VectorStoreQueryResu | |||
|
|||
# If limit_offset_clause is not specified, add TOP clause | |||
if pre_filter is None or pre_filter.get("limit_offset_clause") is None: | |||
query += "TOP @limit " | |||
# query += "TOP @limit " | |||
query += f"TOP {params.get('k', 2)} " |
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.
Generally if you are making a change, we just remove the old code, don't comment it out
@@ -325,25 +325,31 @@ def _query(self, query: VectorStoreQuery, **kwargs: Any) -> VectorStoreQueryResu | |||
|
|||
# If limit_offset_clause is not specified, add TOP clause | |||
if pre_filter is None or pre_filter.get("limit_offset_clause") is None: | |||
query += "TOP @limit " | |||
# query += "TOP @limit " | |||
query += f"TOP {params.get('k', 2)} " |
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.
Also curious, can you give some explanation on this change? This seems to be changing a lot of the query text
|
||
# Add limit_offset_clause if specified | ||
if pre_filter is not None and pre_filter.get("limit_offset_clause") is not None: | ||
query += " {}".format(pre_filter["limit_offset_clause"]) | ||
parameters = [ | ||
{"name": "@limit", "value": params["k"]}, | ||
{"name": "@embeddingKey", "value": self._embedding_key}, | ||
# {"name": "@limit", "value": params["k"]}, |
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 remove the limit param?
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.
Use clear limit value in query, we dont need to pass @limit as parameter
{"name": "@limit", "value": params["k"]}, | ||
{"name": "@embeddingKey", "value": self._embedding_key}, | ||
# {"name": "@limit", "value": params["k"]}, | ||
# {"name": "@embeddingKey", "value": self._embedding_key}, | ||
{"name": "@embeddings", "value": params["vector"]}, |
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.
params["vector"] is not the same as self._embedding_key? looking at line 316 above
params: Dict[str, Any] = {
"vector": query.query_embedding,
"path": self._embedding_key,
"k": query.similarity_top_k,
}
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.
you cannot use the parameter @embeddingKey for container field name in the query. in the query we need to use clear field name c.embeding instead of c.@embeddingKey
@limit will process default 2 if there is no value
Fix the CosmosNoSQL query parameter and update dependencies version llama-index-core = "^0.12.0"
Fixes # (17384) - #17384
New Package?
Did I fill in the
tool.llamahub
section in thepyproject.toml
and provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.toml
file of the package I am updating? (Except for thellama-index-core
package)Type of Change
Please delete options that are not relevant.
How Has This Been Tested?
Your pull-request will likely not be merged unless it is covered by some form of impactful unit testing.
Suggested Checklist:
make format; make lint
to appease the lint gods