From 487060f12862d2312d6a9929bd55fc71d22e20d9 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Mon, 17 Apr 2023 00:04:37 -0700 Subject: [PATCH 1/3] Remove paginate operator during logical plan optimization. Signed-off-by: MaxKsyunz --- .../optimizer/rule/CreatePagingTableScanBuilder.java | 2 +- .../sql/planner/optimizer/LogicalPlanOptimizerTest.java | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/org/opensearch/sql/planner/optimizer/rule/CreatePagingTableScanBuilder.java b/core/src/main/java/org/opensearch/sql/planner/optimizer/rule/CreatePagingTableScanBuilder.java index 3785945374..c635400c33 100644 --- a/core/src/main/java/org/opensearch/sql/planner/optimizer/rule/CreatePagingTableScanBuilder.java +++ b/core/src/main/java/org/opensearch/sql/planner/optimizer/rule/CreatePagingTableScanBuilder.java @@ -67,6 +67,6 @@ public LogicalPlan apply(LogicalPaginate plan, Captures captures) { var scan = logicalRelation.getTable().createPagedScanBuilder(plan.getPageSize()); relationParent.replaceChildPlans(List.of(scan)); - return plan; + return plan.getChild().get(0); } } diff --git a/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java b/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java index 2083fdef9c..2201e81254 100644 --- a/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java @@ -342,13 +342,13 @@ public PhysicalPlan implement(LogicalPlan plan) { () -> table.createWriteBuilder(null)); } - @Test + @Test void paged_table_scan_builder_support_project_push_down_can_apply_its_rule() { var relation = relation("schema", table); assertEquals( - paginate(project(pagedTableScanBuilder), 4), + project(pagedTableScanBuilder), LogicalPlanOptimizer.create().optimize(paginate(project(relation), 4))); } @@ -391,7 +391,6 @@ void push_page_size_noop_if_no_sub_plans() { assertEquals(paginate, LogicalPlanOptimizer.create().optimize(paginate)); } - @Test void table_scan_builder_support_offset_push_down_can_apply_its_rule() { when(table.createPagedScanBuilder(anyInt())).thenReturn(pagedTableScanBuilder); @@ -401,7 +400,7 @@ void table_scan_builder_support_offset_push_down_can_apply_its_rule() { .optimize(new LogicalPaginate(42, List.of(project(relation)))); // `optimized` structure: LogicalPaginate -> LogicalProject -> TableScanBuilder // LogicalRelation replaced by a TableScanBuilder instance - assertEquals(paginate(project(pagedTableScanBuilder), 42), optimized); + assertEquals(project(pagedTableScanBuilder), optimized); } private LogicalPlan optimize(LogicalPlan plan) { From 245a9584ae2f97495c3763a08c58ce9dd143447d Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Mon, 17 Apr 2023 10:10:28 -0700 Subject: [PATCH 2/3] Fix checkstyle issue. Signed-off-by: MaxKsyunz --- .../sql/planner/optimizer/LogicalPlanOptimizerTest.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java b/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java index 2201e81254..543b261d9e 100644 --- a/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/optimizer/LogicalPlanOptimizerTest.java @@ -342,7 +342,7 @@ public PhysicalPlan implement(LogicalPlan plan) { () -> table.createWriteBuilder(null)); } - @Test + @Test void paged_table_scan_builder_support_project_push_down_can_apply_its_rule() { var relation = relation("schema", table); @@ -391,6 +391,7 @@ void push_page_size_noop_if_no_sub_plans() { assertEquals(paginate, LogicalPlanOptimizer.create().optimize(paginate)); } + @Test void table_scan_builder_support_offset_push_down_can_apply_its_rule() { when(table.createPagedScanBuilder(anyInt())).thenReturn(pagedTableScanBuilder); From f8c2fd88133ce2ecabe7198f6cbe3eb95e8f6ff2 Mon Sep 17 00:00:00 2001 From: MaxKsyunz Date: Mon, 17 Apr 2023 11:22:03 -0700 Subject: [PATCH 3/3] Remove PaginateOperator class since it is no longer used. Signed-off-by: MaxKsyunz --- .../sql/planner/DefaultImplementor.java | 7 -- .../planner/physical/PaginateOperator.java | 74 -------------- .../physical/PhysicalPlanNodeVisitor.java | 4 - .../sql/planner/DefaultImplementorTest.java | 8 -- .../physical/PaginateOperatorTest.java | 99 ------------------- .../physical/PhysicalPlanNodeVisitorTest.java | 8 -- .../OpenSearchExecutionProtector.java | 7 -- .../OpenSearchExecutionEngineTest.java | 47 +-------- .../OpenSearchExecutionProtectorTest.java | 8 -- 9 files changed, 1 insertion(+), 261 deletions(-) delete mode 100644 core/src/main/java/org/opensearch/sql/planner/physical/PaginateOperator.java delete mode 100644 core/src/test/java/org/opensearch/sql/planner/physical/PaginateOperatorTest.java diff --git a/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java b/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java index 607a5af983..9bde4ab647 100644 --- a/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java +++ b/core/src/main/java/org/opensearch/sql/planner/DefaultImplementor.java @@ -12,7 +12,6 @@ import org.opensearch.sql.planner.logical.LogicalFilter; import org.opensearch.sql.planner.logical.LogicalLimit; import org.opensearch.sql.planner.logical.LogicalNested; -import org.opensearch.sql.planner.logical.LogicalPaginate; import org.opensearch.sql.planner.logical.LogicalPlan; import org.opensearch.sql.planner.logical.LogicalPlanNodeVisitor; import org.opensearch.sql.planner.logical.LogicalProject; @@ -29,7 +28,6 @@ import org.opensearch.sql.planner.physical.FilterOperator; import org.opensearch.sql.planner.physical.LimitOperator; import org.opensearch.sql.planner.physical.NestedOperator; -import org.opensearch.sql.planner.physical.PaginateOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.ProjectOperator; import org.opensearch.sql.planner.physical.RareTopNOperator; @@ -134,11 +132,6 @@ public PhysicalPlan visitLimit(LogicalLimit node, C context) { return new LimitOperator(visitChild(node, context), node.getLimit(), node.getOffset()); } - @Override - public PhysicalPlan visitPaginate(LogicalPaginate plan, C context) { - return new PaginateOperator(visitChild(plan, context), plan.getPageSize()); - } - @Override public PhysicalPlan visitTableScanBuilder(TableScanBuilder plan, C context) { return plan.build(); diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PaginateOperator.java b/core/src/main/java/org/opensearch/sql/planner/physical/PaginateOperator.java deleted file mode 100644 index 7601f7006a..0000000000 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PaginateOperator.java +++ /dev/null @@ -1,74 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - -package org.opensearch.sql.planner.physical; - -import java.util.List; -import lombok.EqualsAndHashCode; -import lombok.Getter; -import lombok.RequiredArgsConstructor; -import org.opensearch.sql.data.model.ExprValue; -import org.opensearch.sql.executor.ExecutionEngine; -import org.opensearch.sql.planner.SerializablePlan; - -@EqualsAndHashCode(callSuper = false) -@RequiredArgsConstructor -public class PaginateOperator extends PhysicalPlan implements SerializablePlan { - @Getter - private final PhysicalPlan input; - - @Getter - private final int pageSize; - - /** - * Which page is this? - * May not be necessary in the end. Currently used to increment the "cursor counter" -- - * See usage. - */ - @Getter - private int pageIndex = 0; - - private int numReturned = 0; - - /** - * Page given physical plan, with pageSize elements per page, starting with the given page. - */ - public PaginateOperator(PhysicalPlan input, int pageSize, int pageIndex) { - this.pageSize = pageSize; - this.input = input; - this.pageIndex = pageIndex; - } - - @Override - public R accept(PhysicalPlanNodeVisitor visitor, C context) { - return visitor.visitPaginate(this, context); - } - - @Override - public boolean hasNext() { - return numReturned < pageSize && input.hasNext(); - } - - @Override - public ExprValue next() { - numReturned += 1; - return input.next(); - } - - public List getChild() { - return List.of(input); - } - - @Override - public ExecutionEngine.Schema schema() { - return input.schema(); - } - - /** No need to serialize a PaginateOperator, it actually does nothing - it is a wrapper. */ - @Override - public SerializablePlan getPlanForSerialization() { - return (SerializablePlan) input; - } -} diff --git a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java index bc4c0404c4..cb488700a0 100644 --- a/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java +++ b/core/src/main/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitor.java @@ -92,8 +92,4 @@ public R visitAD(PhysicalPlan node, C context) { public R visitML(PhysicalPlan node, C context) { return visitNode(node, context); } - - public R visitPaginate(PaginateOperator node, C context) { - return visitNode(node, context); - } } diff --git a/core/src/test/java/org/opensearch/sql/planner/DefaultImplementorTest.java b/core/src/test/java/org/opensearch/sql/planner/DefaultImplementorTest.java index 768ab27931..bf1464f5f6 100644 --- a/core/src/test/java/org/opensearch/sql/planner/DefaultImplementorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/DefaultImplementorTest.java @@ -58,7 +58,6 @@ import org.opensearch.sql.planner.logical.LogicalPlan; import org.opensearch.sql.planner.logical.LogicalPlanDSL; import org.opensearch.sql.planner.logical.LogicalRelation; -import org.opensearch.sql.planner.physical.PaginateOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.PhysicalPlanDSL; import org.opensearch.sql.storage.Table; @@ -247,11 +246,4 @@ public TableWriteOperator build(PhysicalPlan child) { }; assertEquals(tableWriteOperator, logicalPlan.accept(implementor, null)); } - - @Test - public void visitPaginate_should_build_PaginateOperator_and_keep_page_size() { - var paginate = new LogicalPaginate(42, List.of(values())); - var plan = paginate.accept(implementor, null); - assertEquals(paginate.getPageSize(), ((PaginateOperator) plan).getPageSize()); - } } diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/PaginateOperatorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/PaginateOperatorTest.java deleted file mode 100644 index 2405700f10..0000000000 --- a/core/src/test/java/org/opensearch/sql/planner/physical/PaginateOperatorTest.java +++ /dev/null @@ -1,99 +0,0 @@ -/* - * Copyright OpenSearch Contributors - * SPDX-License-Identifier: Apache-2.0 - */ - - -package org.opensearch.sql.planner.physical; - -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNull; -import static org.junit.jupiter.api.Assertions.assertSame; -import static org.junit.jupiter.api.Assertions.assertThrows; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.mockito.Mockito.CALLS_REAL_METHODS; -import static org.mockito.Mockito.doNothing; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.times; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; -import static org.mockito.Mockito.withSettings; -import static org.opensearch.sql.data.type.ExprCoreType.INTEGER; -import static org.opensearch.sql.data.type.ExprCoreType.STRING; -import static org.opensearch.sql.planner.physical.PhysicalPlanDSL.project; - -import org.junit.jupiter.api.DisplayNameGeneration; -import org.junit.jupiter.api.DisplayNameGenerator; -import org.junit.jupiter.api.Test; -import org.opensearch.sql.data.model.ExprIntegerValue; -import org.opensearch.sql.expression.DSL; -import org.opensearch.sql.planner.SerializablePlan; - -@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class) -public class PaginateOperatorTest extends PhysicalPlanTestBase { - - @Test - public void accept() { - var visitor = new PhysicalPlanNodeVisitor() {}; - assertNull(new PaginateOperator(null, 42).accept(visitor, null)); - } - - @Test - public void hasNext_a_page() { - var plan = mock(PhysicalPlan.class); - when(plan.hasNext()).thenReturn(true); - when(plan.next()).thenReturn(new ExprIntegerValue(42)).thenReturn(null); - var paginate = new PaginateOperator(plan, 1, 1); - assertTrue(paginate.hasNext()); - assertEquals(42, paginate.next().integerValue()); - paginate.next(); - assertFalse(paginate.hasNext()); - assertNull(paginate.next()); - } - - @Test - public void hasNext_no_more_entries() { - var plan = mock(PhysicalPlan.class); - when(plan.hasNext()).thenReturn(false); - var paginate = new PaginateOperator(plan, 1, 1); - assertFalse(paginate.hasNext()); - } - - @Test - public void getChild() { - var plan = mock(PhysicalPlan.class); - var paginate = new PaginateOperator(plan, 1); - assertSame(plan, paginate.getChild().get(0)); - } - - @Test - public void open() { - var plan = mock(PhysicalPlan.class); - doNothing().when(plan).open(); - new PaginateOperator(plan, 1).open(); - verify(plan, times(1)).open(); - } - - @Test - public void schema() { - PhysicalPlan project = project(null, - DSL.named("response", DSL.ref("response", INTEGER)), - DSL.named("action", DSL.ref("action", STRING), "act")); - assertEquals(project.schema(), new PaginateOperator(project, 42).schema()); - } - - @Test - public void schema_assert() { - var plan = mock(PhysicalPlan.class, withSettings().defaultAnswer(CALLS_REAL_METHODS)); - assertThrows(Throwable.class, () -> new PaginateOperator(plan, 42).schema()); - } - - @Test - // PaginateOperator implements SerializablePlan, but not being serialized - public void serializable_but_not_serialized() { - var plan = mock(PhysicalPlan.class, withSettings().extraInterfaces(SerializablePlan.class)); - var paginate = new PaginateOperator(plan, 1, 1); - assertSame(plan, paginate.getPlanForSerialization()); - } -} diff --git a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java index 2e6ce64ac6..fb687277ce 100644 --- a/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java +++ b/core/src/test/java/org/opensearch/sql/planner/physical/PhysicalPlanNodeVisitorTest.java @@ -168,14 +168,6 @@ public void test_visitML() { assertNull(physicalPlanNodeVisitor.visitML(plan, null)); } - @Test - public void test_visitPaginate() { - PhysicalPlanNodeVisitor physicalPlanNodeVisitor = - new PhysicalPlanNodeVisitor() {}; - - assertNull(physicalPlanNodeVisitor.visitPaginate(new PaginateOperator(plan, 42), null)); - } - public static class PhysicalPlanPrinter extends PhysicalPlanNodeVisitor { public String print(PhysicalPlan node) { diff --git a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java index c46b0231a2..9d71cee8c9 100644 --- a/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java +++ b/opensearch/src/main/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtector.java @@ -17,7 +17,6 @@ import org.opensearch.sql.planner.physical.FilterOperator; import org.opensearch.sql.planner.physical.LimitOperator; import org.opensearch.sql.planner.physical.NestedOperator; -import org.opensearch.sql.planner.physical.PaginateOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.ProjectOperator; import org.opensearch.sql.planner.physical.RareTopNOperator; @@ -65,12 +64,6 @@ public PhysicalPlan visitRename(RenameOperator node, Object context) { return new RenameOperator(visitInput(node.getInput(), context), node.getMapping()); } - @Override - public PhysicalPlan visitPaginate(PaginateOperator node, Object context) { - return new PaginateOperator(visitInput(node.getInput(), context), node.getPageSize(), - node.getPageIndex()); - } - /** * Decorate with {@link ResourceMonitorPlan}. */ diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java index ae7319a223..c96782abea 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/OpenSearchExecutionEngineTest.java @@ -50,10 +50,8 @@ import org.opensearch.sql.opensearch.client.OpenSearchClient; import org.opensearch.sql.opensearch.data.value.OpenSearchExprValueFactory; import org.opensearch.sql.opensearch.executor.protector.OpenSearchExecutionProtector; -import org.opensearch.sql.opensearch.request.OpenSearchRequestBuilder; import org.opensearch.sql.opensearch.storage.scan.OpenSearchIndexScan; import org.opensearch.sql.planner.SerializablePlan; -import org.opensearch.sql.planner.physical.PaginateOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.storage.TableScanOperator; import org.opensearch.sql.storage.split.Split; @@ -120,7 +118,7 @@ void execute_with_cursor() { List expected = Arrays.asList( tupleValue(of("name", "John", "age", 20)), tupleValue(of("name", "Allen", "age", 30))); - FakePaginatePlan plan = new FakePaginatePlan(new FakePhysicalPlan(expected.iterator()), 10, 0); + var plan = new FakePhysicalPlan(expected.iterator()); when(protector.protect(plan)).thenReturn(plan); OpenSearchExecutionEngine executor = new OpenSearchExecutionEngine(client, protector, @@ -255,49 +253,6 @@ public void onFailure(Exception e) { assertTrue(plan.hasClosed); } - private static class FakePaginatePlan extends PaginateOperator { - private final PhysicalPlan input; - private final int pageSize; - private final int pageIndex; - - public FakePaginatePlan(PhysicalPlan input, int pageSize, int pageIndex) { - super(input, pageSize, pageIndex); - this.input = input; - this.pageSize = pageSize; - this.pageIndex = pageIndex; - } - - @Override - public void open() { - input.open(); - } - - @Override - public void close() { - input.close(); - } - - @Override - public void add(Split split) { - input.add(split); - } - - @Override - public boolean hasNext() { - return input.hasNext(); - } - - @Override - public ExprValue next() { - return input.next(); - } - - @Override - public ExecutionEngine.Schema schema() { - return input.schema(); - } - } - @RequiredArgsConstructor private static class FakePhysicalPlan extends TableScanOperator implements SerializablePlan { private final Iterator it; diff --git a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java index fd52d08381..fe0077914e 100644 --- a/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java +++ b/opensearch/src/test/java/org/opensearch/sql/opensearch/executor/protector/OpenSearchExecutionProtectorTest.java @@ -63,7 +63,6 @@ import org.opensearch.sql.opensearch.setting.OpenSearchSettings; import org.opensearch.sql.opensearch.storage.scan.OpenSearchIndexScan; import org.opensearch.sql.planner.physical.NestedOperator; -import org.opensearch.sql.planner.physical.PaginateOperator; import org.opensearch.sql.planner.physical.PhysicalPlan; import org.opensearch.sql.planner.physical.PhysicalPlanDSL; @@ -335,13 +334,6 @@ public void testVisitNested() { executionProtector.visitNested(nestedOperator, values(emptyList()))); } - @Test - public void visitPaginate() { - var paginate = new PaginateOperator(values(List.of()), 42); - assertEquals(executionProtector.protect(paginate), - executionProtector.visitPaginate(paginate, null)); - } - PhysicalPlan resourceMonitor(PhysicalPlan input) { return new ResourceMonitorPlan(input, resourceMonitor); }