rishabhdaim commented on code in PR #3143:
URL: https://github.com/apache/jackrabbit-oak/pull/3143#discussion_r4093542858
##########
oak-search-mongot/src/main/java/org/apache/jackrabbit/oak/plugins/index/mongot/query/MongotIndex.java:
##########
@@ -218,8 +223,26 @@ private Cursor streamingCursor(MongoCollection<Document>
collection,
MongotResultAdapter.excerpts(result,
request.excerptColumns()),
null, null);
}, documents::close);
+ // Use a query-specific $count aggregation instead of rows::getSize:
the latter
+ // would fully drain the streaming cursor (in aggregateCursor()'s
batches) the
+ // moment anything calls Cursor.getSize()/JCR's RowIterator.getSize()
- a common
+ // "N results found" UI pattern - even when the caller never intends
to iterate
+ // all rows. $count is cheap (server-side count, no document bodies
returned)
+ // and, unlike the unused getSizeEstimator(IndexPlan) override above,
reflects
+ // this query's actual filter rather than the whole index's document
count.
+ SizeEstimator sizeEstimator = () -> countMatches(collection, pipeline,
definition);
return new FulltextPathCursor(rows, NEVER_REWOUND, plan,
- plan.getFilter().getQueryLimits(), rows::getSize);
+ plan.getFilter().getQueryLimits(), sizeEstimator);
+ }
+
+ private static long countMatches(MongoCollection<Document> collection,
+ List<Document> pipeline,
+ MongotIndexDefinition definition) {
+ List<Document> countPipeline = new ArrayList<>(pipeline);
+ countPipeline.add(new Document("$count", "n"));
Review Comment:
๐ **P2** ยท ๐ฏ Correctness
**๐ Issue:** `$count` runs before `transformPath`, duplicate suppression,
and `shouldInclude`; a relative-property plan can therefore map several Mongo
hits to one returned Oak path, making exact size larger than iteration.
**๐ ๏ธ Fix:** Use the server-side count only for one-to-one plans; otherwise
count accepted paths through the same rules, for example:
```java
Set<String> paths = new HashSet<>();
long count = 0;
while (cursor.hasNext()) {
String path = getPlanResult(plan).transformPath(
cursor.next().getString(MongoFieldNames.PATH));
if (path != null && paths.add(path) && shouldInclude(path, plan)) {
count++;
}
}
return count;
```
Please add a regression case where multiple relative-property hits transform
to the same parent and assert `getSize()` equals the iterated row count.
##########
oak-search-mongot/src/main/java/org/apache/jackrabbit/oak/plugins/index/mongot/query/MongotIndex.java:
##########
@@ -218,8 +223,26 @@ private Cursor streamingCursor(MongoCollection<Document>
collection,
MongotResultAdapter.excerpts(result,
request.excerptColumns()),
null, null);
}, documents::close);
+ // Use a query-specific $count aggregation instead of rows::getSize:
the latter
+ // would fully drain the streaming cursor (in aggregateCursor()'s
batches) the
+ // moment anything calls Cursor.getSize()/JCR's RowIterator.getSize()
- a common
+ // "N results found" UI pattern - even when the caller never intends
to iterate
+ // all rows. $count is cheap (server-side count, no document bodies
returned)
+ // and, unlike the unused getSizeEstimator(IndexPlan) override above,
reflects
+ // this query's actual filter rather than the whole index's document
count.
+ SizeEstimator sizeEstimator = () -> countMatches(collection, pipeline,
definition);
Review Comment:
๐ **P2** ยท ๐ง Resource lifecycle
**๐ Issue:** The result `MongoCursor` is opened before this estimator is
returned, but a size-only caller never advances `rows`, so `documents::close`
is never reached and the result cursor/first batch remains open.
**๐ ๏ธ Fix:** Make `MongotResultIterator` initialize its source on first row
access, e.g. pass `() -> aggregateCursor(collection, pipeline, definition)` and
keep a nullable realized cursor that is closed on exhaustion/failure. Add a
test that calls only `getSize()` and verifies the result-cursor supplier was
not invoked.
##########
oak-search-mongot/src/main/java/org/apache/jackrabbit/oak/plugins/index/mongot/query/MongotIndex.java:
##########
@@ -218,8 +223,26 @@ private Cursor streamingCursor(MongoCollection<Document>
collection,
MongotResultAdapter.excerpts(result,
request.excerptColumns()),
null, null);
}, documents::close);
+ // Use a query-specific $count aggregation instead of rows::getSize:
the latter
+ // would fully drain the streaming cursor (in aggregateCursor()'s
batches) the
+ // moment anything calls Cursor.getSize()/JCR's RowIterator.getSize()
- a common
+ // "N results found" UI pattern - even when the caller never intends
to iterate
+ // all rows. $count is cheap (server-side count, no document bodies
returned)
+ // and, unlike the unused getSizeEstimator(IndexPlan) override above,
reflects
+ // this query's actual filter rather than the whole index's document
count.
+ SizeEstimator sizeEstimator = () -> countMatches(collection, pipeline,
definition);
Review Comment:
๐ก **P3** ยท โก Performance
**๐ Issue:** `FulltextPathCursor` uses `estimatedSize != 0` as its cache
sentinel, so this estimator is invoked again after every zero result and each
call issues another aggregation.
**๐ ๏ธ Fix:** Cache initialization separately in `FulltextPathCursor`:
```java
private boolean sizeEstimated;
if (!sizeEstimated) {
estimatedSize = sizeEstimator.getSize();
sizeEstimated = true;
}
return estimatedSize;
```
A regression test should call exact `getSize()` twice on an empty result and
assert one count aggregation.
##########
oak-search-mongot/src/main/java/org/apache/jackrabbit/oak/plugins/index/mongot/query/MongotIndex.java:
##########
@@ -218,8 +223,26 @@ private Cursor streamingCursor(MongoCollection<Document>
collection,
MongotResultAdapter.excerpts(result,
request.excerptColumns()),
null, null);
}, documents::close);
+ // Use a query-specific $count aggregation instead of rows::getSize:
the latter
+ // would fully drain the streaming cursor (in aggregateCursor()'s
batches) the
+ // moment anything calls Cursor.getSize()/JCR's RowIterator.getSize()
- a common
+ // "N results found" UI pattern - even when the caller never intends
to iterate
+ // all rows. $count is cheap (server-side count, no document bodies
returned)
+ // and, unlike the unused getSizeEstimator(IndexPlan) override above,
reflects
+ // this query's actual filter rather than the whole index's document
count.
+ SizeEstimator sizeEstimator = () -> countMatches(collection, pipeline,
definition);
return new FulltextPathCursor(rows, NEVER_REWOUND, plan,
- plan.getFilter().getQueryLimits(), rows::getSize);
+ plan.getFilter().getQueryLimits(), sizeEstimator);
+ }
+
+ private static long countMatches(MongoCollection<Document> collection,
+ List<Document> pipeline,
+ MongotIndexDefinition definition) {
+ List<Document> countPipeline = new ArrayList<>(pipeline);
+ countPipeline.add(new Document("$count", "n"));
Review Comment:
๐ **P2** ยท โก Performance
**๐ Issue:** Cloning the result pipeline carries `$sort` into `$count`;
sorting does not change cardinality, but it is a blocking stage and can make an
ordered exact-size request process and buffer the full ordered set.
**๐ ๏ธ Fix:** Build a count-specific pipeline that keeps search/filter stages
and the required sort-field `$match`, but drops result-only `$sort` and the
final projection before appending `$count`. At minimum, remove top-level
`$sort` stages from the copied pipeline and add a test that inspects the
ordered-query count pipeline.
##########
oak-search-mongot/src/main/java/org/apache/jackrabbit/oak/plugins/index/mongot/query/MongotIndex.java:
##########
@@ -268,9 +291,30 @@ private static MongoCursor<Document>
aggregateCursor(MongoCollection<Document> c
+
TimeUnit.MILLISECONDS.toNanos(SEARCH_READINESS_TIMEOUT_MILLIS);
while (true) {
try {
- int[] fetchSizes = definition.getQueryFetchSizes();
+ // Each driver batch is a full round-trip to the server, so
the batch
+ // size should be as large as the configuration allows: using
the
+ // smallest configured fetch size here previously turned a
query with
+ // many hits into hundreds of round-trips and dominated query
latency
+ // (e.g. ~18s for ~2000 hits at fetchSizes[0]=10, vs. <1s at
1000; the
+ // MongoDB driver holds one batch at a time, and the server
caps a
+ // single batch response at 16MB regardless of the requested
size, so
+ // this is safe rather than causing unbounded per-batch
memory/time).
+ //
+ // NOTE / follow-up optimization opportunity (out of scope
here): this
+ // always uses the largest configured size, even for queries
that only
+ // need a handful of rows (e.g. a small LIMIT / typeahead
query), which
+ // wastes server/network work building an oversized first
batch. A
+ // properly adaptive scheme (small first batch, growing only
if the
+ // caller keeps pulling) needs a row-limit hint to be threaded
through
+ // from the query plan down to here - that hint doesn't
currently exist
+ // in the backend-neutral IndexPlan/Filter/FulltextIndex SPI
(oak-search)
+ // that this and the Elastic/Lucene backends all implement, so
it isn't
+ // something this module can fix alone. Worth revisiting as a
shared,
+ // implementation-neutral improvement to
FulltextIndex/IndexPlan rather
+ // than a mongot-only special case.
+ int batchSize =
Arrays.stream(definition.getQueryFetchSizes()).max().getAsInt();
Review Comment:
๐ **P2** ยท ๐ Compatibility
**๐ Issue:** Existing definitions treat `queryFetchSizes` as the
Elastic-compatible sequence, but this changes every Mongot initial batch to its
maximum; custom definitions that intentionally start small silently change
behavior.
**๐ ๏ธ Fix:** Add a scalar Mongot `queryBatchSize` and use
`definition.getQueryBatchSize()` here. For compatibility, preserve
`queryFetchSizes[0]` when a legacy custom sequence is present and the new
scalar is absent; the new default can still be `1000` for definitions using
defaults.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
