Count optimization changes #811
Conversation
| @@ -100,6 +100,23 @@ public interface OperationsConfig { | |||
| @WithDefault("1000") | |||
| int maxVectorSearchLimit(); | |||
|
|
|||
| /** | |||
| * @return Maximum size of keys read from database to return count, Setting it to -1 will use | |||
There was a problem hiding this comment.
Maximum size -> Maximum number ?
src/main/java/io/stargate/sgv2/jsonapi/config/OperationsConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/config/OperationsConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/config/OperationsConfig.java
Outdated
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/CountOperation.java
Show resolved
Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/CountOperation.java
Outdated
Show resolved
Hide resolved
| @@ -38,16 +55,29 @@ private SimpleStatement buildSelectQuery() { | |||
| if (expressions != null && !expressions.isEmpty() && expressions.get(0) != null) { | |||
| collect = ExpressionBuilder.getExpressionValuesInOrder(expressions.get(0)); | |||
There was a problem hiding this comment.
I know this is existing unchanged code, but can expressions ever have more than 1 element? It seems that'd not be supported. Would it make sense to assert (throw exception) if more than one exists?
There was a problem hiding this comment.
Not sure why this is done that way. I will create a issue to handle all expressions,
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/ReadOperation.java
Outdated
Show resolved
Hide resolved
| @@ -5,9 +5,13 @@ | |||
| import java.util.Map; | |||
| import java.util.function.Supplier; | |||
|
|
|||
| public record CountOperationPage(long count) implements Supplier<CommandResult> { | |||
| public record CountOperationPage(long count, boolean moreData) implements Supplier<CommandResult> { | |||
There was a problem hiding this comment.
We can use moreData, but could also name it something like countIncomplete or hasMoreRows?
There was a problem hiding this comment.
Used it because DeleteMany and UpdateMany use this falg.
There was a problem hiding this comment.
Looks good -- functionality fine, only suggestions are wrt naming and documentation/comments (to help whoever reads code).
The only substantial suggestion other than that is whether we should throw JsonApiException from failing query. That seems like it could be useful for debugging.
| @@ -335,7 +351,7 @@ public void withExistFalseOperator() { | |||
| .post(CollectionResource.BASE_PATH, namespaceName, collectionName) | |||
| .then() | |||
| .statusCode(200) | |||
| .body("status.count", is(4)) | |||
| .body("status.count", is(5)) | |||
There was a problem hiding this comment.
Why did this change? Doesn't seem like a perf optimization should change the result here
There was a problem hiding this comment.
@vkarpov15 Have inserted one more document for testing moreData flag. Because of the additional record count increased by 1 for the provided filter condition in this test case.
There was a problem hiding this comment.
Makes sense, thanks for clarifying.
What this PR does:
Count command changes to read primary key data and count in the client side. Number of keys read is controlled by configuration
Which issue(s) this PR fixes:
Fixes #809
Checklist