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

Changes for indexing option - create collection and cache #771

Merged

maheshrajamani merged 6 commits into main from indexing-options-meta-data

Jan 8, 2024

Contributor

maheshrajamani commented Jan 5, 2024 •

edited

Loading

What this PR does:
Added the option to provide fields for indexing - allowed or denied
Add the indexing option to cache

Which issue(s) this PR fixes:
Fixes #765 #766

Checklist

Changes manually tested
Automated Tests added/updated
Documentation added/updated
CLA Signed: DataStax CLA


          Changes for indexing option - create collection and cache

cf63111

maheshrajamani requested a review from a team as a code owner

January 5, 2024 16:59

tatu-at-datastax reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/CollectionSettings.java Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

...va/io/stargate/sgv2/jsonapi/service/resolver/model/impl/CreateCollectionCommandResolver.java Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java Outdated

+                              type = SchemaType.ARRAY,
+                              implementation = String.class)
+                          @Nullable
+                          List<String> deny) {}

Contributor

tatu-at-datastax Jan 5, 2024

As per my note below, I think validation could be encapsulated within config class itself; either in (canonical) constructor, or, maybe cleaner, in a create(...) method that verifies everything is good (and if not throws exception), then calls canonical constructor.

Contributor Author

maheshrajamani Jan 5, 2024

Thought about it., For this need to create separate validators to verify things like either one of the field can only be used. So decided to do it as part of resolver.

Contributor

tatu-at-datastax Jan 5, 2024

By validator I didn't mean anything pluggable or related to Quarkus.

tatu-at-datastax reviewed

View reviewed changes

...va/io/stargate/sgv2/jsonapi/service/resolver/model/impl/CreateCollectionCommandResolver.java Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

Contributor

tatu-at-datastax left a comment

Looks good overall, added some suggestions for refactoring.

maheshrajamani added 2 commits

January 5, 2024 13:16


          Moved the indexing validation code inside the IndexingConfig

a4800cf


          Merge remote-tracking branch 'origin/main' into indexing-options-meta…

e875b51

…-data

maheshrajamani self-assigned this

Contributor

tatu-at-datastax commented Jan 5, 2024

Ok, I'll read through everything once more (in a bit, offline for an hour), to make sure I get the overall thing right.
I think I missed some aspects.

tatu-at-datastax reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/exception/ErrorCode.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/service/cqldriver/executor/CollectionSettings.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

...o/stargate/sgv2/jsonapi/service/resolver/model/impl/CreateCollectionCommandResolverTest.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed

View reviewed changes

...o/stargate/sgv2/jsonapi/service/resolver/model/impl/CreateCollectionCommandResolverTest.java Outdated Show resolved Hide resolved

tatu-at-datastax approved these changes

View reviewed changes

Contributor

tatu-at-datastax left a comment

Ok looks good, thank you for taking time to explain where I misunderstood code & constraints.

Added some suggestions as usual; nothing major.

kathirsvn reviewed

View reviewed changes

src/main/java/io/stargate/sgv2/jsonapi/api/model/command/impl/CreateCollectionCommand.java Outdated Show resolved Hide resolved

kathirsvn reviewed

View reviewed changes

src/test/java/io/stargate/sgv2/jsonapi/api/v1/CreateCollectionIntegrationTest.java Show resolved Hide resolved

maheshrajamani added 3 commits

January 8, 2024 10:27


          Changes based on review comments

db0e4ba


          Merge remote-tracking branch 'origin/main' into indexing-options-meta…

17ad2aa

…-data


          Renamed error code

7d11d17

tatu-at-datastax approved these changes

View reviewed changes

maheshrajamani merged commit 587a6b9 into main

2 checks passed

maheshrajamani deleted the indexing-options-meta-data branch

January 8, 2024 16:24

maheshrajamani restored the indexing-options-meta-data branch

January 8, 2024 17:07

maheshrajamani deleted the indexing-options-meta-data branch

January 8, 2024 17:07

vkarpov15 reviewed

View reviewed changes

Collaborator

vkarpov15 left a comment

LGTM 👍

Hazel-Datastax mentioned this pull request

Error out when using non indexed fields #781

Merged

4 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet