From c23e4d418e8c9fa64cfa05508dcb1c415d5a3855 Mon Sep 17 00:00:00 2001 From: Yury-Fridlyand Date: Mon, 29 May 2023 21:36:49 -0700 Subject: [PATCH 1/2] Remove `getTotalHits` feature. Signed-off-by: Yury-Fridlyand --- .../sql/executor/ExecutionEngine.java | 1 - .../planner/physical/CursorCloseOperator.java | 6 -- .../sql/planner/physical/FilterOperator.java | 8 --- .../sql/planner/physical/NestedOperator.java | 9 --- .../sql/planner/physical/PhysicalPlan.java | 11 --- .../sql/planner/physical/ValuesOperator.java | 7 -- .../sql/executor/QueryServiceTest.java | 2 +- .../MicroBatchStreamingExecutionTest.java | 2 +- .../physical/CursorCloseOperatorTest.java | 9 --- .../planner/physical/FilterOperatorTest.java | 18 ----- .../planner/physical/NestedOperatorTest.java | 6 -- .../planner/physical/PhysicalPlanTest.java | 17 ----- .../planner/physical/ValuesOperatorTest.java | 2 - .../sql/executor/DefaultExecutionEngine.java | 2 +- docs/dev/Pagination-v2.md | 71 +++++-------------- .../sql/legacy/plugin/RestSQLQueryAction.java | 2 +- .../executor/OpenSearchExecutionEngine.java | 2 +- .../protector/ResourceMonitorPlan.java | 5 -- .../response/OpenSearchResponse.java | 4 -- .../storage/scan/OpenSearchIndexScan.java | 6 -- .../system/OpenSearchSystemIndexScan.java | 10 +-- .../executor/ResourceMonitorPlanTest.java | 6 -- .../response/OpenSearchResponseTest.java | 4 -- .../storage/scan/OpenSearchIndexScanTest.java | 22 ++---- .../system/OpenSearchSystemIndexScanTest.java | 1 - .../transport/TransportPPLQueryAction.java | 2 +- .../opensearch/sql/ppl/PPLServiceTest.java | 6 +- .../sql/protocol/response/QueryResult.java | 6 +- .../format/JdbcResponseFormatter.java | 2 +- .../protocol/response/QueryResultTest.java | 10 +-- .../format/CommandResponseFormatterTest.java | 2 +- .../format/JdbcResponseFormatterTest.java | 4 +- 32 files changed, 45 insertions(+), 220 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java index 8d87bd9b146..9465da22c91 100644 --- a/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java +++ b/core/src/main/java/org/opensearch/sql/executor/ExecutionEngine.java @@ -54,7 +54,6 @@ void execute(PhysicalPlan plan, ExecutionContext context, class QueryResponse { private final Schema schema; private final List results; - private final long total; private final Cursor cursor; } diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/CursorCloseOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/CursorCloseOperator.java index 13a37fb61ea..7921d0dd507 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/CursorCloseOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/CursorCloseOperator.java @@ -49,12 +49,6 @@ public ExecutionEngine.Schema schema() { return new ExecutionEngine.Schema(List.of()); } - // TODO remove - @Override - public long getTotalHits() { - return 0; - } - @Override public void open() { // no-op, no search should be invoked. diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/FilterOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/FilterOperator.java index a9c7597c3e5..4b5045d24e1 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/FilterOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/FilterOperator.java @@ -32,7 +32,6 @@ public class FilterOperator extends PhysicalPlan { private final Expression conditions; @ToString.Exclude private ExprValue next = null; - private long totalHits = 0; @Override public R accept(PhysicalPlanNodeVisitor visitor, C context) { @@ -51,7 +50,6 @@ public boolean hasNext() { ExprValue exprValue = conditions.valueOf(inputValue.bindingTuples()); if (!(exprValue.isNull() || exprValue.isMissing()) && (exprValue.booleanValue())) { next = inputValue; - totalHits++; return true; } } @@ -62,10 +60,4 @@ public boolean hasNext() { public ExprValue next() { return next; } - - @Override - public long getTotalHits() { - // ignore `input.getTotalHits()`, because it returns wrong (unfiltered) value - return totalHits; - } } diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/NestedOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/NestedOperator.java index cea8ae6c141..54cd541519c 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/NestedOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/NestedOperator.java @@ -47,8 +47,6 @@ public class NestedOperator extends PhysicalPlan { @EqualsAndHashCode.Exclude private ListIterator> flattenedResult = result.listIterator(); - private long totalHits = 0; - /** * Constructor for NestedOperator with list of map as arg. * @param input : PhysicalPlan input. @@ -121,13 +119,11 @@ public ExprValue next() { if (result.isEmpty()) { flattenedResult = result.listIterator(); - totalHits++; return new ExprTupleValue(new LinkedHashMap<>()); } flattenedResult = result.listIterator(); } - totalHits++; return new ExprTupleValue(new LinkedHashMap<>(flattenedResult.next())); } @@ -283,9 +279,4 @@ private void getNested( row, ret, currentObj); } } - - @Override - public long getTotalHits() { - return totalHits; - } } diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java index b4547a63b06..247b347940d 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlan.java @@ -44,15 +44,4 @@ public ExecutionEngine.Schema schema() { throw new IllegalStateException(String.format("[BUG] schema can been only applied to " + "ProjectOperator, instead of %s", this.getClass().getSimpleName())); } - - /** - * Returns Total hits matched the search criteria. Note: query may return less if limited. - * {@see Settings#QUERY_SIZE_LIMIT}. - * Any plan which adds/removes rows to the response should overwrite it to provide valid values. - * - * @return Total hits matched the search criteria. - */ - public long getTotalHits() { - return getChild().stream().mapToLong(PhysicalPlan::getTotalHits).max().orElse(0); - } } diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/ValuesOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/ValuesOperator.java index 45884830e10..4ac9d6a30a6 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/ValuesOperator.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/ValuesOperator.java @@ -56,13 +56,6 @@ public boolean hasNext() { return valuesIterator.hasNext(); } - @Override - public long getTotalHits() { - // ValuesOperator used for queries without `FROM` clause, e.g. `select 1`. - // Such query always returns 1 row. - return 1; - } - @Override public ExprValue next() { List values = valuesIterator.next().stream() diff --git a/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java b/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java index 525de79afca..1510b304e6e 100644 --- a/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java +++ b/core/src/test/java/org/opensearch/sql/executor/QueryServiceTest.java @@ -133,7 +133,7 @@ Helper executeSuccess(Split split) { invocation -> { ResponseListener listener = invocation.getArgument(2); listener.onResponse( - new ExecutionEngine.QueryResponse(schema, Collections.emptyList(), 0, + new ExecutionEngine.QueryResponse(schema, Collections.emptyList(), Cursor.None)); return null; }) diff --git a/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java b/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java index ceb53b756a5..f0974db13ea 100644 --- a/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java +++ b/core/src/test/java/org/opensearch/sql/executor/streaming/MicroBatchStreamingExecutionTest.java @@ -170,7 +170,7 @@ Helper executeSuccess(Long... offsets) { ResponseListener listener = invocation.getArgument(2); listener.onResponse( - new ExecutionEngine.QueryResponse(null, Collections.emptyList(), 0, + new ExecutionEngine.QueryResponse(null, Collections.emptyList(), Cursor.None)); PlanContext planContext = invocation.getArgument(1); diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/CursorCloseOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/CursorCloseOperatorTest.java index 66111c1042f..5ae30faa307 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/CursorCloseOperatorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/CursorCloseOperatorTest.java @@ -27,15 +27,6 @@ public void never_hasNext() { assertFalse(plan.hasNext()); } - // TODO remove - @Test - public void no_total_hits() { - var plan = new CursorCloseOperator(null); - assertEquals(0, plan.getTotalHits()); - plan.open(); - assertEquals(0, plan.getTotalHits()); - } - @Test public void open_is_not_propagated() { var child = mock(PhysicalPlan.class); diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/FilterOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/FilterOperatorTest.java index 247cfe6a1de..6a8bcad2034 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/FilterOperatorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/FilterOperatorTest.java @@ -50,7 +50,6 @@ public void filter_test() { .tupleValue(ImmutableMap .of("ip", "209.160.24.63", "action", "GET", "response", 404, "referer", "www.amazon.com")))); - assertEquals(1, plan.getTotalHits()); } @Test @@ -64,7 +63,6 @@ public void null_value_should_been_ignored() { DSL.equal(DSL.ref("response", INTEGER), DSL.literal(404))); List result = execute(plan); assertEquals(0, result.size()); - assertEquals(0, plan.getTotalHits()); } @Test @@ -78,21 +76,5 @@ public void missing_value_should_been_ignored() { DSL.equal(DSL.ref("response", INTEGER), DSL.literal(404))); List result = execute(plan); assertEquals(0, result.size()); - assertEquals(0, plan.getTotalHits()); - } - - @Test - public void totalHits() { - when(inputPlan.hasNext()).thenReturn(true, true, true, true, true, false); - var answers = Stream.of(200, 240, 300, 403, 404).map(c -> - new ExprTupleValue(new LinkedHashMap<>(Map.of("response", new ExprIntegerValue(c))))) - .collect(Collectors.toList()); - when(inputPlan.next()).thenAnswer(AdditionalAnswers.returnsElementsOf(answers)); - - FilterOperator plan = new FilterOperator(inputPlan, - DSL.less(DSL.ref("response", INTEGER), DSL.literal(400))); - List result = execute(plan); - assertEquals(3, result.size()); - assertEquals(3, plan.getTotalHits()); } } diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/NestedOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/NestedOperatorTest.java index 9024ae50c96..5f8bf99b0df 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/NestedOperatorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/NestedOperatorTest.java @@ -162,7 +162,6 @@ public void nested_one_nested_field() { ) ) ); - assertEquals(3, nested.getTotalHits()); } @Test @@ -241,7 +240,6 @@ public void nested_two_nested_field() { ) ) ); - assertEquals(9, nested.getTotalHits()); } @Test @@ -284,7 +282,6 @@ public void nested_two_nested_fields_with_same_path() { ) ) ); - assertEquals(3, nested.getTotalHits()); } @Test @@ -304,7 +301,6 @@ public void non_nested_field_tests() { tupleValue(new LinkedHashMap<>(Map.of("message", "val"))) ) ); - assertEquals(1, nested.getTotalHits()); } @Test @@ -323,7 +319,6 @@ public void nested_missing_tuple_field() { tupleValue(new LinkedHashMap<>(Map.of("message.val", ExprNullValue.of()))) ) ); - assertEquals(1, nested.getTotalHits()); } @Test @@ -340,6 +335,5 @@ public void nested_missing_array_field() { .get(0) .tupleValue() .size()); - assertEquals(1, nested.getTotalHits()); } } diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanTest.java index 2c67994d2ec..ab3f0ef36d5 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanTest.java @@ -61,21 +61,4 @@ void add_split_to_child_by_default() { testPlan.add(split); verify(child).add(split); } - - @Test - void get_total_hits_from_child() { - var plan = mock(PhysicalPlan.class); - when(child.getTotalHits()).thenReturn(42L); - when(plan.getChild()).thenReturn(List.of(child)); - when(plan.getTotalHits()).then(CALLS_REAL_METHODS); - assertEquals(42, plan.getTotalHits()); - verify(child).getTotalHits(); - } - - @Test - void get_total_hits_uses_default_value() { - var plan = mock(PhysicalPlan.class); - when(plan.getTotalHits()).then(CALLS_REAL_METHODS); - assertEquals(0, plan.getTotalHits()); - } } diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/ValuesOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/ValuesOperatorTest.java index bf6d28a23c6..9acab03d2bd 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/ValuesOperatorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/ValuesOperatorTest.java @@ -9,7 +9,6 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.empty; -import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.is; import static org.opensearch.sql.data.model.ExprValueUtils.collectionValue; import static org.opensearch.sql.expression.DSL.literal; @@ -45,7 +44,6 @@ public void iterateSingleRow() { results, contains(collectionValue(Arrays.asList(1, "abc"))) ); - assertThat(values.getTotalHits(), equalTo(1L)); } } diff --git a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java index 3849d686a6a..db72498a1d3 100644 --- a/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java +++ b/core/src/testFixtures/java/org/opensearch/sql/executor/DefaultExecutionEngine.java @@ -34,7 +34,7 @@ public void execute( result.add(plan.next()); } QueryResponse response = new QueryResponse(new Schema(new ArrayList<>()), new ArrayList<>(), - 0, Cursor.None); + Cursor.None); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/docs/dev/Pagination-v2.md b/docs/dev/Pagination-v2.md index f80b95fae7d..361ff68248d 100644 --- a/docs/dev/Pagination-v2.md +++ b/docs/dev/Pagination-v2.md @@ -450,8 +450,8 @@ SQLService ->>+ QueryPlanFactory: execute Processing of an Initial Query Request has few extra steps comparing versus processing a regular Query Request: 1. Query validation with `CanPaginateVisitor`. This is required to validate whether incoming query can be paged. This also activate legacy engine fallback mechanism. -2. `Serialization` is performed by `PlanSerializer` - it converts Physical Query Plan into a cursor, which could be used query a next page. - +2. Creating a paged index scan with `CreatePagingTableScanBuilder` `Optimizer` rule. A Regular Query Request triggers `CreateTableScanBuilder` rule instead. +3. `Serialization` is performed by `PlanSerializer` - it converts Physical Plan Tree into a cursor, which could be used query a next page. ```mermaid sequenceDiagram @@ -482,9 +482,6 @@ SQLService ->>+ QueryPlanFactory : execute OpenSearchExecutionEngine ->>+ PlanSerializer : convertToCursor PlanSerializer -->>- OpenSearchExecutionEngine : cursor end - rect rgb(91, 123, 155) - Note over OpenSearchExecutionEngine : get total hits - end OpenSearchExecutionEngine -->>- QueryService : execution completed QueryService -->>- QueryPlanFactory : execution completed QueryPlanFactory -->>- SQLService : execution completed @@ -493,10 +490,9 @@ SQLService ->>+ QueryPlanFactory : execute #### Subsequent Query Request Subsequent pages are processed by a new workflow. The key point there: -1. `Deserialization` is performed by `PlanSerializer` to restore entire Physical Query Plan encoded into the cursor. -2. Since query already contains the Physical Query Plan, analysis and optimization steps are no-ops. -3. `Serialization` is performed by `PlanSerializer` - it converts Physical Query Plan into a cursor, which could be used query a next page. -4. Traversal of Physical Query Plan to get total hits, which is required to properly fill response to a user. +1. `Deserialization` is performed by `PlanSerializer` to restore entire Physical Plan Tree encoded into the cursor. +2. Since query already contains the Physical Plan Tree, all tree processing steps are skipped. +3. `Serialization` is performed by `PlanSerializer` - it converts Physical Plan Tree into a cursor, which could be used query a next page. ```mermaid sequenceDiagram @@ -509,17 +505,20 @@ sequenceDiagram SQLService ->>+ QueryPlanFactory : execute QueryPlanFactory ->>+ QueryService : execute - QueryService ->>+ Analyzer : analyze - Analyzer -->>- QueryService : new LogicalFetchCursor - QueryService ->>+ Planner : plan - Planner ->>+ DefaultImplementor : implement - DefaultImplementor ->>+ PlanSerializer : deserialize - PlanSerializer -->>- DefaultImplementor: physical query plan - DefaultImplementor -->>- Planner : physical query plan - Planner -->>- QueryService : physical query plan - QueryService ->>+ OpenSearchExecutionEngine : execute - OpenSearchExecutionEngine -->>- QueryService: execution completed - QueryService -->>- QueryPlanFactory : execution completed + rect rgb(91, 123, 155) + note over QueryService, PlanSerializer : Deserialization + QueryService ->>+ PlanSerializer: convertToPlan + PlanSerializer -->>- QueryService: Physical plan tree + end + Note over QueryService : Planner, Optimizer and Implementor
are skipped + QueryService ->>+ OpenSearchExecutionEngine : execute + rect rgb(91, 123, 155) + note over OpenSearchExecutionEngine, PlanSerializer : Serialization + OpenSearchExecutionEngine ->>+ PlanSerializer : convertToCursor + PlanSerializer -->>- OpenSearchExecutionEngine : cursor + end + OpenSearchExecutionEngine -->>- QueryService: execution completed + QueryService -->>- QueryPlanFactory : execution completed QueryPlanFactory -->>- SQLService : execution completed ``` @@ -776,35 +775,3 @@ class PhysicalPlan: def close: innerPlan.close() ``` - -#### Total Hits - -Total Hits is the number of rows matching the search criteria; with `select *` queries it is equal to row (doc) number in the table (index). -Example: -Paging thru `SELECT * FROM calcs` (17 rows) with `fetch_size = 5` returns: - -* Page 1: total hits = 17, result size = 5, cursor -* Page 2: total hits = 17, result size = 5, cursor -* Page 3: total hits = 17, result size = 5, cursor -* Page 4: total hits = 17, result size = 2, cursor -* Page 5: total hits = 0, result size = 0 - -Default implementation of `getTotalHits` in a Physical Plan iterate child plans down the tree and gets the maximum value or 0. - -```mermaid -sequenceDiagram - participant OpenSearchExecutionEngine - participant ProjectOperator - participant ResourceMonitorPlan - participant OpenSearchIndexScan - -OpenSearchExecutionEngine ->>+ ProjectOperator: getTotalHits - Note over ProjectOperator: default implementation - ProjectOperator ->>+ ResourceMonitorPlan: getTotalHits - Note over ResourceMonitorPlan: call to delegate - ResourceMonitorPlan ->>+ OpenSearchIndexScan: getTotalHits - Note over OpenSearchIndexScan: use stored value from the search response - OpenSearchIndexScan -->>- ResourceMonitorPlan: value - ResourceMonitorPlan -->>- ProjectOperator: value - ProjectOperator -->>- OpenSearchExecutionEngine: value -``` diff --git a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java index a432c2f473b..37cbba4adfd 100644 --- a/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java +++ b/legacy/src/main/java/org/opensearch/sql/legacy/plugin/RestSQLQueryAction.java @@ -180,7 +180,7 @@ private ResponseListener createQueryResponseListener( public void onResponse(QueryResponse response) { sendResponse(channel, OK, formatter.format(new QueryResult(response.getSchema(), response.getResults(), - response.getCursor(), response.getTotal()))); + response.getCursor()))); } @Override diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java index 0f32e4a2ee0..31e5c7f957c 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngine.java @@ -52,7 +52,7 @@ public void execute(PhysicalPlan physicalPlan, ExecutionContext context, } QueryResponse response = new QueryResponse(physicalPlan.schema(), result, - plan.getTotalHits(), planSerializer.convertToCursor(plan)); + planSerializer.convertToCursor(plan)); listener.onResponse(response); } catch (Exception e) { listener.onFailure(e); diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/ResourceMonitorPlan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/ResourceMonitorPlan.java index 0ec4d743b31..4c02affc5e1 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/ResourceMonitorPlan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/ResourceMonitorPlan.java @@ -87,11 +87,6 @@ public ExprValue next() { return delegate.next(); } - @Override - public long getTotalHits() { - return delegate.getTotalHits(); - } - @Override public SerializablePlan getPlanForSerialization() { return (SerializablePlan) delegate; diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java index af43be1a383..733fad6203e 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/response/OpenSearchResponse.java @@ -96,10 +96,6 @@ public boolean isEmpty() { return (hits.getHits() == null) || (hits.getHits().length == 0) && aggregations == null; } - public long getTotalHits() { - return hits.getTotalHits().value; - } - public boolean isAggregationResponse() { return aggregations != null; } diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java index 3633e45449a..e216e1e2fec 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScan.java @@ -88,12 +88,6 @@ public ExprValue next() { return iterator.next(); } - @Override - public long getTotalHits() { - // ignore response.getTotalHits(), because response returns entire index, regardless of LIMIT - return queryCount; - } - private void fetchNextBatch() { OpenSearchResponse response = client.search(request); if (!response.isEmpty()) { diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScan.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScan.java index eba5eb126de..ee377263c15 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScan.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScan.java @@ -22,7 +22,7 @@ @ToString(onlyExplicitlyIncluded = true) public class OpenSearchSystemIndexScan extends TableScanOperator { /** - * OpenSearch client. + * OpenSearch request. */ private final OpenSearchSystemRequest request; @@ -31,12 +31,9 @@ public class OpenSearchSystemIndexScan extends TableScanOperator { */ private Iterator iterator; - private long totalHits = 0; - @Override public void open() { var response = request.search(); - totalHits = response.size(); iterator = response.iterator(); } @@ -50,11 +47,6 @@ public ExprValue next() { return iterator.next(); } - @Override - public long getTotalHits() { - return totalHits; - } - @Override public String explain() { return request.toString(); diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/ResourceMonitorPlanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/ResourceMonitorPlanTest.java index 0b9f302ceba..96e85a81738 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/ResourceMonitorPlanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/ResourceMonitorPlanTest.java @@ -111,12 +111,6 @@ void acceptSuccess() { verify(plan, times(1)).accept(visitor, context); } - @Test - void getTotalHitsSuccess() { - monitorPlan.getTotalHits(); - verify(plan, times(1)).getTotalHits(); - } - @Test void getPlanForSerialization() { plan = mock(PhysicalPlan.class, withSettings().extraInterfaces(SerializablePlan.class)); diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java index 8add6c8c856..079a82b7830 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/response/OpenSearchResponseTest.java @@ -82,27 +82,23 @@ void isEmpty() { var response = new OpenSearchResponse(searchResponse, factory, includes); assertFalse(response.isEmpty()); - assertEquals(2L, response.getTotalHits()); when(searchResponse.getHits()).thenReturn(SearchHits.empty()); when(searchResponse.getAggregations()).thenReturn(null); response = new OpenSearchResponse(searchResponse, factory, includes); assertTrue(response.isEmpty()); - assertEquals(0L, response.getTotalHits()); when(searchResponse.getHits()) .thenReturn(new SearchHits(null, new TotalHits(0, TotalHits.Relation.EQUAL_TO), 0)); response = new OpenSearchResponse(searchResponse, factory, includes); assertTrue(response.isEmpty()); - assertEquals(0L, response.getTotalHits()); when(searchResponse.getHits()).thenReturn(SearchHits.empty()); when(searchResponse.getAggregations()).thenReturn(new Aggregations(emptyList())); response = new OpenSearchResponse(searchResponse, factory, includes); assertFalse(response.isEmpty()); - assertEquals(0L, response.getTotalHits()); } @Test diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java index 0f39f635a79..08590f8021c 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/scan/OpenSearchIndexScanTest.java @@ -149,10 +149,7 @@ void query_empty_result() { try (OpenSearchIndexScan indexScan = new OpenSearchIndexScan(client, QUERY_SIZE, requestBuilder.build(name, MAX_RESULT_WINDOW, CURSOR_KEEP_ALIVE))) { indexScan.open(); - assertAll( - () -> assertFalse(indexScan.hasNext()), - () -> assertEquals(0, indexScan.getTotalHits()) - ); + assertFalse(indexScan.hasNext()); } verify(client).cleanup(any()); } @@ -179,8 +176,7 @@ void query_all_results_with_query() { () -> assertTrue(indexScan.hasNext()), () -> assertEquals(employee(3, "Allen", "IT"), indexScan.next()), - () -> assertFalse(indexScan.hasNext()), - () -> assertEquals(3, indexScan.getTotalHits()) + () -> assertFalse(indexScan.hasNext()) ); } verify(client).cleanup(any()); @@ -210,8 +206,7 @@ void query_all_results_with_scroll() { () -> assertTrue(indexScan.hasNext()), () -> assertEquals(employee(3, "Allen", "IT"), indexScan.next()), - () -> assertFalse(indexScan.hasNext()), - () -> assertEquals(3, indexScan.getTotalHits()) + () -> assertFalse(indexScan.hasNext()) ); } verify(client).cleanup(any()); @@ -241,8 +236,7 @@ void query_some_results_with_query() { () -> assertTrue(indexScan.hasNext()), () -> assertEquals(employee(3, "Allen", "IT"), indexScan.next()), - () -> assertFalse(indexScan.hasNext()), - () -> assertEquals(3, indexScan.getTotalHits()) + () -> assertFalse(indexScan.hasNext()) ); } verify(client).cleanup(any()); @@ -266,8 +260,7 @@ void query_some_results_with_scroll() { () -> assertTrue(indexScan.hasNext()), () -> assertEquals(employee(3, "Allen", "IT"), indexScan.next()), - () -> assertFalse(indexScan.hasNext()), - () -> assertEquals(3, indexScan.getTotalHits()) + () -> assertFalse(indexScan.hasNext()) ); } verify(client).cleanup(any()); @@ -300,8 +293,7 @@ void query_results_limited_by_query_size() { () -> assertTrue(indexScan.hasNext()), () -> assertEquals(employee(2, "Smith", "HR"), indexScan.next()), - () -> assertFalse(indexScan.hasNext()), - () -> assertEquals(2, indexScan.getTotalHits()) + () -> assertFalse(indexScan.hasNext()) ); } verify(client).cleanup(any()); @@ -429,8 +421,6 @@ public OpenSearchResponse answer(InvocationOnMock invocation) { when(response.isEmpty()).thenReturn(false); ExprValue[] searchHit = searchHitBatches[batchNum]; when(response.iterator()).thenReturn(Arrays.asList(searchHit).iterator()); - lenient().when(response.getTotalHits()) - .thenReturn((long) searchHitBatches[batchNum].length); } else { when(response.isEmpty()).thenReturn(true); } diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScanTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScanTest.java index c04ef25611e..494f3ff2d0e 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScanTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/storage/system/OpenSearchSystemIndexScanTest.java @@ -32,7 +32,6 @@ public void queryData() { systemIndexScan.open(); assertTrue(systemIndexScan.hasNext()); assertEquals(stringValue("text"), systemIndexScan.next()); - assertEquals(1, systemIndexScan.getTotalHits()); } @Test diff --git a/plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java b/plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java index acac65bd54f..dbe5230abf6 100644 --- a/plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java +++ b/plugin/src/main/java/org/opensearch/sql/plugin/transport/TransportPPLQueryAction.java @@ -140,7 +140,7 @@ private ResponseListener createListener( public void onResponse(ExecutionEngine.QueryResponse response) { String responseContent = formatter.format(new QueryResult(response.getSchema(), response.getResults(), - response.getCursor(), response.getTotal())); + response.getCursor())); listener.onResponse(new TransportPPLQueryResponse(responseContent)); } diff --git a/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java b/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java index 74e5b0f82ea..c14eb3dba16 100644 --- a/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java +++ b/ppl/src/test/java/org/opensearch/sql/ppl/PPLServiceTest.java @@ -67,7 +67,7 @@ public void cleanup() throws InterruptedException { public void testExecuteShouldPass() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), 0, Cursor.None)); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None)); return null; }).when(queryService).execute(any(), any()); @@ -89,7 +89,7 @@ public void onFailure(Exception e) { public void testExecuteCsvFormatShouldPass() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), 0, Cursor.None)); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None)); return null; }).when(queryService).execute(any(), any()); @@ -163,7 +163,7 @@ public void onFailure(Exception e) { public void testPrometheusQuery() { doAnswer(invocation -> { ResponseListener listener = invocation.getArgument(1); - listener.onResponse(new QueryResponse(schema, Collections.emptyList(), 0, Cursor.None)); + listener.onResponse(new QueryResponse(schema, Collections.emptyList(), Cursor.None)); return null; }).when(queryService).execute(any(), any()); diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java index d06dba7719f..ae663644197 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/QueryResult.java @@ -36,12 +36,8 @@ public class QueryResult implements Iterable { @Getter private final Cursor cursor; - @Getter - private final long total; - - public QueryResult(ExecutionEngine.Schema schema, Collection exprValues) { - this(schema, exprValues, Cursor.None, exprValues.size()); + this(schema, exprValues, Cursor.None); } /** diff --git a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatter.java b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatter.java index b9a2d2fcc64..1ad3ffde344 100644 --- a/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatter.java +++ b/protocol/src/main/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatter.java @@ -40,7 +40,7 @@ protected Object buildJsonObject(QueryResult response) { json.datarows(fetchDataRows(response)); // Populate other fields - json.total(response.getTotal()) + json.total(response.size()) .size(response.size()) .status(200); if (!response.getCursor().equals(Cursor.None)) { diff --git a/protocol/src/test/java/org/opensearch/sql/protocol/response/QueryResultTest.java b/protocol/src/test/java/org/opensearch/sql/protocol/response/QueryResultTest.java index 470bb205a80..4c58e189b89 100644 --- a/protocol/src/test/java/org/opensearch/sql/protocol/response/QueryResultTest.java +++ b/protocol/src/test/java/org/opensearch/sql/protocol/response/QueryResultTest.java @@ -36,7 +36,7 @@ void size() { tupleValue(ImmutableMap.of("name", "John", "age", 20)), tupleValue(ImmutableMap.of("name", "Allen", "age", 30)), tupleValue(ImmutableMap.of("name", "Smith", "age", 40)) - ), Cursor.None, 0); + ), Cursor.None); assertEquals(3, response.size()); } @@ -46,7 +46,7 @@ void columnNameTypes() { schema, Collections.singletonList( tupleValue(ImmutableMap.of("name", "John", "age", 20)) - ), Cursor.None, 0); + ), Cursor.None); assertEquals( ImmutableMap.of("name", "string", "age", "integer"), @@ -61,7 +61,7 @@ void columnNameTypesWithAlias() { QueryResult response = new QueryResult( schema, Collections.singletonList(tupleValue(ImmutableMap.of("n", "John"))), - Cursor.None, 0); + Cursor.None); assertEquals( ImmutableMap.of("n", "string"), @@ -73,7 +73,7 @@ void columnNameTypesWithAlias() { void columnNameTypesFromEmptyExprValues() { QueryResult response = new QueryResult( schema, - Collections.emptyList(), Cursor.None, 0); + Collections.emptyList(), Cursor.None); assertEquals( ImmutableMap.of("name", "string", "age", "integer"), response.columnNameTypes() @@ -102,7 +102,7 @@ void iterate() { Arrays.asList( tupleValue(ImmutableMap.of("name", "John", "age", 20)), tupleValue(ImmutableMap.of("name", "Allen", "age", 30)) - ), Cursor.None, 0); + ), Cursor.None); int i = 0; for (Object[] objects : response) { diff --git a/protocol/src/test/java/org/opensearch/sql/protocol/response/format/CommandResponseFormatterTest.java b/protocol/src/test/java/org/opensearch/sql/protocol/response/format/CommandResponseFormatterTest.java index 17bd8aee8d0..a3052324fe5 100644 --- a/protocol/src/test/java/org/opensearch/sql/protocol/response/format/CommandResponseFormatterTest.java +++ b/protocol/src/test/java/org/opensearch/sql/protocol/response/format/CommandResponseFormatterTest.java @@ -42,7 +42,7 @@ public void produces_always_same_output_for_any_query_response() { .put("address", "Seattle") .put("age", 20) .build())), - new Cursor("test_cursor"), 42); + new Cursor("test_cursor")); assertEquals("{\n" + " \"succeeded\": true\n" diff --git a/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatterTest.java b/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatterTest.java index 047e297c266..9c79b1bf89b 100644 --- a/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatterTest.java +++ b/protocol/src/test/java/org/opensearch/sql/protocol/response/format/JdbcResponseFormatterTest.java @@ -97,7 +97,7 @@ void format_response_with_cursor() { .put("address", "Seattle") .put("age", 20) .build())), - new Cursor("test_cursor"), 42); + new Cursor("test_cursor")); assertJsonEquals( "{" @@ -108,7 +108,7 @@ void format_response_with_cursor() { + "]," + "\"datarows\":[" + "[\"John\",\"Seattle\",20]]," - + "\"total\":42," + + "\"total\":1," + "\"size\":1," + "\"cursor\":\"test_cursor\"," + "\"status\":200}", From 71d57a2899121ae0bb5b829a0f11dda04d1716f8 Mon Sep 17 00:00:00 2001 From: Yury-Fridlyand Date: Tue, 30 May 2023 08:20:16 -0700 Subject: [PATCH 2/2] Fix doc after merge. Signed-off-by: Yury-Fridlyand --- docs/dev/Pagination-v2.md | 35 ++++++++++++++++------------------- 1 file changed, 16 insertions(+), 19 deletions(-) diff --git a/docs/dev/Pagination-v2.md b/docs/dev/Pagination-v2.md index 361ff68248d..1c3510b116d 100644 --- a/docs/dev/Pagination-v2.md +++ b/docs/dev/Pagination-v2.md @@ -450,8 +450,7 @@ SQLService ->>+ QueryPlanFactory: execute Processing of an Initial Query Request has few extra steps comparing versus processing a regular Query Request: 1. Query validation with `CanPaginateVisitor`. This is required to validate whether incoming query can be paged. This also activate legacy engine fallback mechanism. -2. Creating a paged index scan with `CreatePagingTableScanBuilder` `Optimizer` rule. A Regular Query Request triggers `CreateTableScanBuilder` rule instead. -3. `Serialization` is performed by `PlanSerializer` - it converts Physical Plan Tree into a cursor, which could be used query a next page. +2. `Serialization` is performed by `PlanSerializer` - it converts Physical Plan Tree into a cursor, which could be used query a next page. ```mermaid sequenceDiagram @@ -496,30 +495,28 @@ Subsequent pages are processed by a new workflow. The key point there: ```mermaid sequenceDiagram - participant SQLService participant QueryPlanFactory participant QueryService - participant OpenSearchExecutionEngine + participant Analyzer + participant Planner participant DefaultImplementor participant PlanSerializer + participant OpenSearchExecutionEngine -SQLService ->>+ QueryPlanFactory : execute - QueryPlanFactory ->>+ QueryService : execute - rect rgb(91, 123, 155) - note over QueryService, PlanSerializer : Deserialization - QueryService ->>+ PlanSerializer: convertToPlan - PlanSerializer -->>- QueryService: Physical plan tree - end - Note over QueryService : Planner, Optimizer and Implementor
are skipped - QueryService ->>+ OpenSearchExecutionEngine : execute +QueryPlanFactory ->>+ QueryService : execute + QueryService ->>+ Analyzer : analyze + Analyzer -->>- QueryService : new LogicalFetchCursor + QueryService ->>+ Planner : plan + Planner ->>+ DefaultImplementor : implement rect rgb(91, 123, 155) - note over OpenSearchExecutionEngine, PlanSerializer : Serialization - OpenSearchExecutionEngine ->>+ PlanSerializer : convertToCursor - PlanSerializer -->>- OpenSearchExecutionEngine : cursor + DefaultImplementor ->>+ PlanSerializer : deserialize + PlanSerializer -->>- DefaultImplementor: physical query plan end - OpenSearchExecutionEngine -->>- QueryService: execution completed - QueryService -->>- QueryPlanFactory : execution completed - QueryPlanFactory -->>- SQLService : execution completed + DefaultImplementor -->>- Planner : physical query plan + Planner -->>- QueryService : physical query plan + QueryService ->>+ OpenSearchExecutionEngine : execute + OpenSearchExecutionEngine -->>- QueryService: execution completed + QueryService -->>- QueryPlanFactory : execution completed ``` #### Legacy Engine Fallback