Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Count command implementation #117

Merged
merged 14 commits into from
Feb 15, 2023
Merged

Count command implementation #117

merged 14 commits into from
Feb 15, 2023

Conversation

maheshrajamani
Copy link
Contributor

@maheshrajamani maheshrajamani commented Feb 14, 2023 •
edited
Loading

Implementation for #116

FilterableResolver and FindOperation are refactored so it can be used for count command also.

@maheshrajamani maheshrajamani marked this pull request as ready for review February 14, 2023 21:15
@maheshrajamani maheshrajamani requested a review from a team as a code owner February 14, 2023 21:15
vkarpov15
vkarpov15 approved these changes Feb 14, 2023
View reviewed changes
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

ivansenic
ivansenic reviewed Feb 15, 2023
View reviewed changes
Copy link
Contributor

@ivansenic ivansenic left a comment

Choose a reason for hiding this comment

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

I am not convinced with putting everything into a single operation.. And single result, etc. Why are we doing this?

src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/ReadOperation.java Outdated Show resolved Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/ReadOperation.java Outdated Show resolved Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/FindOperation.java Outdated Show resolved Hide resolved
src/main/java/io/stargate/sgv2/jsonapi/service/operation/model/impl/ReadOperationPage.java Outdated Show resolved Hide resolved
tatu-at-datastax
tatu-at-datastax reviewed Feb 15, 2023
View reviewed changes
src/test/java/io/stargate/sgv2/jsonapi/api/v1/CountIntegrationTest.java
@QuarkusIntegrationTest
@TestInstance(TestInstance.Lifecycle.PER_CLASS)
@TestMethodOrder(MethodOrderer.OrderAnnotation.class)
public class CountIntegrationTest extends CollectionResourceBaseIntegrationTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Good job: I like coverage here, seems to include all kinds of filtering cases.

tatu-at-datastax
tatu-at-datastax approved these changes Feb 15, 2023
View reviewed changes
Copy link
Contributor

@tatu-at-datastax tatu-at-datastax left a comment

Choose a reason for hiding this comment

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

Looks good: my only questions are wrt javadocs/documentation, could extend some.

@maheshrajamani maheshrajamani merged commit 294eaa0 into main Feb 15, 2023
@maheshrajamani maheshrajamani deleted the count branch February 15, 2023 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ivansenic ivansenic ivansenic left review comments

@tatu-at-datastax tatu-at-datastax tatu-at-datastax approved these changes

@vkarpov15 vkarpov15 vkarpov15 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@maheshrajamani @vkarpov15 @ivansenic @tatu-at-datastax