Skip to content

Comments

CBL-6416: Regression - VS LIMIT query should throw for out of range#2181

Merged
jianminzhao merged 3 commits intomasterfrom
cbl-6416
Nov 7, 2024
Merged

CBL-6416: Regression - VS LIMIT query should throw for out of range#2181
jianminzhao merged 3 commits intomasterfrom
cbl-6416

Conversation

@jianminzhao
Copy link
Contributor

No description provided.

@cbl-bot
Copy link

cbl-bot commented Nov 5, 2024

Code Coverage Results:

Type Percentage
branches 66.07
functions 78.28
instantiations 70.49
lines 77.42
regions 73.23

@jianminzhao jianminzhao requested a review from snej November 6, 2024 16:35
@snej
Copy link
Collaborator

snej commented Nov 6, 2024

Sorry to derail this, but I don't think the 3.2 behavior is the right thing to do. There's no other case where a non-positive LIMIT causes an error; a zero or negative value just means zero results. And why arbitrarily prevent limits above 10000?

I'm pretty sure this is not consistent with vector query on Server, and we're supposed to be compatible with that.

@jianminzhao jianminzhao requested a review from pasin November 7, 2024 16:55
Copy link
Collaborator

@pasin pasin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For limit 0 and negative, the server returns zero results. We can follow that behavior. I also don't think this is a breaking change as nobody can use limit 0 or negative before in CBL. I will create tickets for the platforms to remove the tests that check error for limit 0 and negative.

For limit 10000 to the vector search, it is a guardrail set by PM so that we have put this in 3.2.0 (It's in the API spec). Can we continue to have this guardrails for 4.0 beta, and we can discuss with PM to remove or change that number after the beta?

@pasin
Copy link
Collaborator

pasin commented Nov 7, 2024

I have created https://jira.issues.couchbase.com/browse/CBL-6426 for the limit 10000.

Add tests with LIMIT = 0 and -1, that query should return 0 results.
@jianminzhao jianminzhao requested a review from pasin November 7, 2024 21:09
@jianminzhao jianminzhao merged commit d2b20e2 into master Nov 7, 2024
@jianminzhao jianminzhao deleted the cbl-6416 branch November 7, 2024 23:31
jianminzhao added a commit that referenced this pull request Nov 11, 2024
In this build has branch release/3.2 merged at c67cbd3 (3.2.1), plus
- Catch2 V3
- CBL-6332 Implicit key change in query results (#2182)
- d2b20e2 CBL-6416: Regression - VS LIMIT query should throw for out of range (#2181)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants