Changes for findOne command #27

maheshrajamani · 2023-01-10T18:55:30Z

The changes also include for find

ivansenic

I am having problem following this.. Can I get a guide @maheshrajamani

P.S. PR should not be this big.. It's almost impossible to review this: +2,628 −22..

ivansenic · 2023-01-12T14:52:46Z

src/main/java/io/stargate/sgv3/docsapi/service/bridge/config/DocumentConfig.java

+  /**
+   * @return Defines the maximum limit of document that can be returned for a request, defaults to
+   *     <code>1000</code>.
+   */
+  @Max(Integer.MAX_VALUE)
+  @Positive
+  @WithDefault("1000")
+  int maxLimit();


how's maxLimit() different from maxPageSize()? If we are storing document per row, why do we need this limit?

This was different in previous versions of docs api, as we stored documents in multiple rows, thus we need to defined limis for documents getting back and limits for page size in the executed queries.. I don't see this situation here?

maxPageSize is for maximum number of record that can be fetched in a query iteration, which can be paginated. maxLimit is maximum number of documents that can be delivered for a command. If a find command (yet to be implemented) reads all the document in a collection, even if it's paginated it will return only max document set as limit.

Ah so this is because of the possible in memory filtering right? 👍

But default of 1000 seems wrong, 20 is the default "page" size in many implementations

It's not in that context. Let say if a query is "select * from table", maxLimit defines how many records we will return irrespective of pagination. If maxLimit is 1000 the query will be run as "select * from table limit 1000" if limit value cannot be resolved from the command.

aha, ok, but shouldn't limit always be same as page size? 😕

It need not be. If for example the user want to show any 200 document on the screen (may be sorted). The limit will be 200. To keep the VM in check in stargate we will have max page size as 100. The driver will paginate and return 200 records.

src/main/java/io/stargate/sgv3/docsapi/service/operation/model/Operation.java

ivansenic · 2023-01-12T14:55:50Z

src/main/java/io/stargate/sgv3/docsapi/service/operation/model/ReadOperation.java

+              int remaining = rSet.getRowsCount();
+              int colCount = rSet.getColumnsCount();
+              List<ReadDocument> documents = new ArrayList<>(remaining);
+              Iterator<QueryOuterClass.Row> rowIterator = rSet.getRowsList().stream().iterator();
+              while (--remaining >= 0 && rowIterator.hasNext()) {
+                QueryOuterClass.Row row = rowIterator.next();
+                ReadDocument document = null;
+                try {
+                  document =
+                      new ReadDocument(
+                          Values.string(row.getValues(0)), // key
+                          Optional.of(Values.uuid(row.getValues(1))), // tx_id
+                          readDocument
+                              ? objectMapper.readTree(Values.string(row.getValues(2)))
+                              : null);
+                } catch (JsonProcessingException e) {
+                  throw new DocsException(ErrorCode.DOCUMENT_UNPARSEABLE);
+                }
+                documents.add(document);
+              }


do we want something similar as we have in the docs v3 -> io.stargate.sgv2.docsapi.service.common.model.RowWrapper

Imo this is a very nice utility that helps you row columns by name.. I see you used ids here, is this what we want to do?

There are only max of 3 static columns read from the table as per design. I don't think we need them.

src/main/java/io/stargate/sgv3/docsapi/service/operation/model/ReadOperation.java

ivansenic · 2023-01-12T14:59:11Z

src/main/java/io/stargate/sgv3/docsapi/service/operation/model/ReadOperation.java

+                              ? objectMapper.readTree(Values.string(row.getValues(2)))
+                              : null);
+                } catch (JsonProcessingException e) {
+                  throw new DocsException(ErrorCode.DOCUMENT_UNPARSEABLE);


this means if parsing any of the rows fails, this ends up in the exception. Is this the wanted behavior?

objectMapper.readTree is throwing JsonProcessingException which needs to be handled but in theory this will never happen because the json document stored is validated during insert.

src/test/java/io/stargate/sgv3/docsapi/api/v3/CollectionResourceIntegrationTest.java

src/test/java/io/stargate/sgv3/docsapi/service/bridge/AbstractValidatingStargateBridgeTest.java

...st/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/FindOneCommandResolverTest.java

maheshrajamani · 2023-01-12T15:53:01Z

P.S. PR should not be this big.. It's almost impossible to review this:

There are 27 files out of which 10 are test class. Even with the remaining most only 8 classes has code logic in it. Not sure how to split it further.

maheshrajamani · 2023-01-12T15:53:33Z

I am having problem following this.. Can I get a guide @maheshrajamani

Let me know if you need a code walk through on whats being done.

Changed the paging state retrieved from resultSet similar to other places.

ivansenic · 2023-01-13T10:52:12Z

P.S. PR should not be this big.. It's almost impossible to review this:

There are 27 files out of which 10 are test class. Even with the remaining most only 8 classes has code logic in it. Not sure how to split it further.

Not sure, but 2.6K new code lines is not something we should do in single PR in future..

ivansenic

Posted some improvements.. Would be great if @amorton looks on this as well, as this is originally his code..

src/main/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/FindOneCommandResolver.java

ivansenic · 2023-01-17T11:38:37Z

src/main/java/io/stargate/sgv3/docsapi/service/operation/model/impl/FindOperation.java

+ */
+public record FindOperation(
+    CommandContext commandContext,
+    List<DBFilterBase> filters,


Yes. I just think these should be taken outside of this class and to a separate file. They are also used outside of the class, in the resolver, so it feels better for me to have a dedicated class/package.

...in/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/matcher/FilterableResolver.java

src/main/java/io/stargate/sgv3/docsapi/service/operation/model/impl/FindOperation.java

ivansenic · 2023-01-17T12:18:38Z

...in/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/matcher/FilterableResolver.java

+        ErrorCode.FILTER_UNRESOLVABLE, "Options need to be returned for filterable of non findOne");
+  }
+
+  private Operation findDynamic(CommandContext commandContext, CaptureGroups<T> captures) {


@amorton @maheshrajamani This also seems implemented wrong.. Seems like you know what captures are gonna be passed to this method and thus you can go through them one by one using markers.. It's kinda defeating the whole purpose of captures..

I would propose, together with removal of the FilterMatchRules to:

define all the available capture groups, we have four now I would add one additional specifically for the _id, so that's handled as well

CaptureGroups does not have to map with a marker, but direct instances of all available groups (nullable)

CaptureGroups expose getters for each group, which are typed correctly Optional<CaptureGroup<String>>> getDynamicTextGroup(), etc

I am also not sure what's stopping us from having:

pair -> filters.add( new FindOperation.TextFilter( pair.path(), FindOperation.MapFilterBase.Operator.EQ, pair.value())));

directly in the text capture group. If this would be the case each capture group can return list of filters. And thus CaptureGroups could even just accept list of capture groups, interate through them collect and join filters and returns this to the user of the class.. we don't even have to expose which groups we have to the outside..

Also note that as soon as you need OR, NOT and other boolean logic, capture groups would be thrown in the garbage, as they can not handle these. Maybe some part of the impl would be usable, but what we will end up having would be something like:

boolean logic around ComparisonExpression

set of rules for simplifications (for example transform NOT(NE(5) => EQ(5)

set of rules that convert each of the ComparisonExpression to the DBFilter or MemoryFilter

...in/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/matcher/FilterableResolver.java

maheshrajamani · 2023-01-17T18:08:31Z

@ivansenic As part of today's standup we discussed about changing the resolver to not use the Match Rules etc and use of library to resole the bool query. This is something we need discussion with @amorton. So suggestion was to create an issue to revisit resolver implementation once the deadline commands are finished for the Demo. Let me know if you have other concerns on the code else we will merge it for the time.

src/main/java/io/stargate/sgv3/docsapi/service/bridge/config/DocumentConfig.java

src/main/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/matcher/CaptureGroup.java

tatu-at-datastax · 2023-01-17T18:57:49Z

src/test/java/io/stargate/sgv3/docsapi/api/v3/CollectionResourceIntegrationTest.java

@@ -56,17 +57,105 @@ public final void createCollection() {
  @Nested
  class FindOne {

+    @BeforeEach


I think it'd make sense to have separate test classes for Insert, FindXxx as these classes may grow quite bit; nested groups can help a bit but maybe not enough.

Yes this class will grow bigger with time I will split it to multiple classes. Can it wait for next PR or need it as part of this?

It can wait no problem.

tatu-at-datastax

Ok it's a big PR and I can see @ivansenic has valid concerns about FilterMatchRules (and related). But as per earlier discussions I also think that we may need to merge things as-is now and clean up at a later step, to avoid being blocked by this PR -- it is difficult to split it into smaller cleaner pieces.

So approving with expectation of issues being tackled as follow-uo steps.

maheshrajamani · 2023-01-17T20:18:33Z

Created issue #33 and #34 to discuss and close the unresolved items. Merging this PR to proceed with other commands.

ivansenic · 2023-01-18T13:14:45Z

@maheshrajamani I see this is merged, but are any of my comments solved? Like that Swagger issue? Raw non parametrized types?

Changes for findOne command

22613f1

The changes also include for find

maheshrajamani marked this pull request as ready for review January 10, 2023 19:07

maheshrajamani requested a review from a team as a code owner January 10, 2023 19:07

maheshrajamani added 2 commits January 11, 2023 11:08

Added java docs

9891589

Changed FindOperation as record

c7ae5c3

ivansenic reviewed Jan 12, 2023

View reviewed changes

maheshrajamani added 9 commits January 12, 2023 12:44

Changes for test classes and removed unused code

6065425

Removed toString() comparison and implemented equals and hashCode

33aa538

Changed the logic to return paging state

2ec110c

Made object mapper a private field in FilterResolver

b91ac84

Changed the paging state retrieved from resultSet similar to other places.

Changed to handle null paging state

7aa86b7

IT test fix

5a6baa5

IT fix to compare json data

d38145d

IT fix to compare json data

5669847

IT fix to compare json data

a490e1a

maheshrajamani marked this pull request as draft January 12, 2023 19:49

maheshrajamani added 4 commits January 12, 2023 14:49

IT fix to compare json data

8a42f3d

IT fix to compare json data

d90184e

IT fix to compare json data

4092d7d

IT fix to compare json data

fa42501

maheshrajamani marked this pull request as ready for review January 12, 2023 20:34

Inject the object mapper as part of the constructor

c8cd641

ivansenic suggested changes Jan 17, 2023

View reviewed changes

tatu-at-datastax reviewed Jan 17, 2023

View reviewed changes

src/main/java/io/stargate/sgv3/docsapi/service/bridge/config/DocumentConfig.java Outdated Show resolved Hide resolved

tatu-at-datastax reviewed Jan 17, 2023

View reviewed changes

src/main/java/io/stargate/sgv3/docsapi/service/resolver/model/impl/matcher/CaptureGroup.java Outdated Show resolved Hide resolved

Changed the DocumentConfig comment

2c3ca82

Changed the name from CapturePair to CaptureExpression

0e1af4d

tatu-at-datastax reviewed Jan 17, 2023

View reviewed changes

tatu-at-datastax approved these changes Jan 17, 2023

View reviewed changes

Fix to handle _id in the path

3eb3db7

maheshrajamani merged commit 45aa7aa into main Jan 17, 2023

maheshrajamani deleted the find/resolver-operation branch January 17, 2023 20:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Changes for findOne command #27

Changes for findOne command #27

maheshrajamani commented Jan 10, 2023

ivansenic left a comment

ivansenic Jan 12, 2023

maheshrajamani Jan 12, 2023

ivansenic Jan 13, 2023 •

edited

Loading

maheshrajamani Jan 13, 2023

ivansenic Jan 16, 2023

maheshrajamani Jan 17, 2023

ivansenic Jan 12, 2023

maheshrajamani Jan 12, 2023

ivansenic Jan 12, 2023

maheshrajamani Jan 12, 2023

maheshrajamani commented Jan 12, 2023

maheshrajamani commented Jan 12, 2023

ivansenic commented Jan 13, 2023

ivansenic left a comment

ivansenic Jan 17, 2023

ivansenic Jan 17, 2023

ivansenic Jan 17, 2023

ivansenic Jan 17, 2023 •

edited

Loading

maheshrajamani commented Jan 17, 2023

tatu-at-datastax Jan 17, 2023

maheshrajamani Jan 17, 2023 •

edited

Loading

tatu-at-datastax Jan 17, 2023

tatu-at-datastax left a comment

maheshrajamani commented Jan 17, 2023

ivansenic commented Jan 18, 2023

Changes for findOne command #27

Changes for findOne command #27

Conversation

maheshrajamani commented Jan 10, 2023

ivansenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivansenic Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maheshrajamani commented Jan 12, 2023

maheshrajamani commented Jan 12, 2023

ivansenic commented Jan 13, 2023

ivansenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivansenic Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

maheshrajamani commented Jan 17, 2023

Choose a reason for hiding this comment

maheshrajamani Jan 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tatu-at-datastax left a comment

Choose a reason for hiding this comment

maheshrajamani commented Jan 17, 2023

ivansenic commented Jan 18, 2023

ivansenic Jan 13, 2023 •

edited

Loading

ivansenic Jan 17, 2023 •

edited

Loading

maheshrajamani Jan 17, 2023 •

edited

Loading