CodeTrainerMan commented on PR #1144: URL: https://github.com/apache/flink-agents/pull/1144#issuecomment-5750977395
Thanks for the review @wenjin272. I went with the first option: require `limit > 0` in both Java and Python (commit 66577096). Reason: handling `0` at the common query entry point would only cover `BaseVectorStore#query(VectorStoreQuery)`. `queryEmbedding(...)` is a public abstract method that can still be called directly with `limit=0`, so on that path the backend inconsistency would remain (Elasticsearch / OpenSearch / S3 Vectors coerce `0` to `1`, Chroma rejects `n_results=0`). Rejecting `0` when the query is constructed also fails fast with a message naming the parameter, rather than silently returning an empty list when `limit` comes from a computed value. Changes: - Java: `VectorStoreQuery` rejects `limit <= 0` (previously `< 0`); the message is now "`limit` must be positive, but was N." - Python: `VectorStoreQuery.limit` uses `gt=0` (previously `ge=0`). - Tests in both languages now expect `limit=0` to be rejected alongside negative values; positive limits keep their current behaviour and the Python default of 10 is unchanged. Verified locally: Java `api` module full test run (449 tests, 0 failures, spotless check included) and the Python limit-validation tests (4 passed). Happy to switch to option B if the community prefers to keep `limit=0` as a legal "return nothing" semantic - in that case I will add the production-path regression test you mentioned. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
