Calcite enable pushdown aggregation by qianheng-aws · Pull Request #3389 · opensearch-project/sql

qianheng-aws · 2025-03-06T10:39:25Z

Description

This PR include changes:

Migrate pushdown process from physical index scan operator to logical index scan operator. The reason is that, in Calcite, some functions(i.e. AVG) only have logical definition while without physical definition or implementation, so we cannot push down such functions for a physical plan.
Enable aggregation pushdown for the general cases
Add VARSAMP and VARPOP implementation.

Related Issues

Resolves #3332

./gradlew :integ-test:integTest --tests '*Calcite*IT' succeed locally.

Check List

New functionality includes testing.
New functionality has been documented.
New functionality has javadoc added.
New functionality has a user manual doc added.
API changes companion pull request created.
Commits are signed per the DCO using --signoff.
Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Heng Qian <qianheng@amazon.com>

LantaoJin · 2025-03-07T01:33:13Z

Please update the latest code and rerun all ITs and UTs locally. PR added and refactored many ITs and UTs.

LantaoJin

I think we still need UT for pushdown features, it shouldn't be blocked by explain feature.
We can add a CalcitePushdownTest with following code for checking.

RelNode finalPlan = Frameworks.getPlanner(getConfig().build()).transform(0, RelTraitSet.createEmpty(), root);
String finalPlanString = RelOptUtil.dumpPlan("", finalPlan, SqlExplainFormat.TEXT, SqlExplainLevel.ALL_ATTRIBUTES);

LantaoJin · 2025-03-07T02:29:24Z

...arch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalOSIndexScan.java

+import org.opensearch.sql.opensearch.storage.OpenSearchIndex;
+
+@Getter
+public class CalciteLogicalOSIndexScan extends CalciteOSIndexScan {


minor: "OS" is not very convenient for searching classes. (It's fine to use it as a variable name). I think we can remove the "OS" in class name, so does in its parent class. We don't have other type of Index. Or keeping the full name "OpenSearch" also acceptable.

LantaoJin · 2025-03-07T02:33:08Z

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteOSIndexScan.java

+import org.opensearch.sql.opensearch.storage.OpenSearchIndex;
+
+/** Relational expression representing a scan of an OpenSearchIndex type. */
+public abstract class CalciteOSIndexScan extends TableScan {


ditto. rename to CalciteOpenSearchIndexScan or CalciteIndexScan

LantaoJin · 2025-03-07T02:36:40Z

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteOSIndexScan.java

+
+    @Override
+    public boolean add(PushDownAction pushDownAction) {
+      // Defense check. I never do push down to this context after aggregate push-down.


penghuo · 2025-03-07T04:14:38Z

opensearch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteOSIndexScan.java

+      PushDownContext pushDownContext) {
+    super(cluster, traitSet, hints, table);
+    this.osIndex = requireNonNull(osIndex, "OpenSearch index");
+    ;


remove it? format checker does not detect it?

Seems "./gradlew spotlessApply" doesn't fix it.

penghuo · 2025-03-07T04:19:51Z

...arch/src/main/java/org/opensearch/sql/opensearch/storage/scan/CalciteLogicalOSIndexScan.java

+      return newScan;
+    } catch (Exception e) {
+      if (LOG.isDebugEnabled()) {
+        LOG.debug("Cannot pushdown the aggregate {}", aggregate, e);


any reasons only add stacktrace in debug log?

There are too many cases where there is unsupported pushdown aggreagte/filter and it will report such issue many times since Calcite will hit this rule many times in its optimizing processes.

Hence, we need to avoid log explosion by removing trackstrace in no debugging mode.

Signed-off-by: Heng Qian <qianheng@amazon.com>

…ine' into feature/calcite-engine-pushdown-agg

qianheng-aws · 2025-03-07T08:34:50Z

I think we still need UT for pushdown features, it shouldn't be blocked by explain feature. We can add a CalcitePushdownTest with following code for checking.
RelNode finalPlan = Frameworks.getPlanner(getConfig().build()).transform(0, RelTraitSet.createEmpty(), root);
String finalPlanString = RelOptUtil.dumpPlan("", finalPlan, SqlExplainFormat.TEXT, SqlExplainLevel.ALL_ATTRIBUTES);

Frameworks.getPlanner is not suitable for this purpose. It has state inner and is designed for planning a query from query string. We cannot directly use its transform API.

May need further investigation or implementation on getting the final optimized plan.

…ine' into feature/calcite-engine-pushdown-agg

* Change push down to logical index scan Signed-off-by: Heng Qian <qianheng@amazon.com> * Support Aggregate Push Down Signed-off-by: Heng Qian <qianheng@amazon.com> * Rebase and resolve conflict Signed-off-by: Heng Qian <qianheng@amazon.com> * Add TODO Signed-off-by: Heng Qian <qianheng@amazon.com> * Address comments Signed-off-by: Heng Qian <qianheng@amazon.com> --------- Signed-off-by: Heng Qian <qianheng@amazon.com>

* Change push down to logical index scan Signed-off-by: Heng Qian <qianheng@amazon.com> * Support Aggregate Push Down Signed-off-by: Heng Qian <qianheng@amazon.com> * Rebase and resolve conflict Signed-off-by: Heng Qian <qianheng@amazon.com> * Add TODO Signed-off-by: Heng Qian <qianheng@amazon.com> * Address comments Signed-off-by: Heng Qian <qianheng@amazon.com> --------- Signed-off-by: Heng Qian <qianheng@amazon.com> Signed-off-by: xinyual <xinyual@amazon.com>

qianheng-aws added 3 commits March 6, 2025 18:17

Change push down to logical index scan

d232e9f

Signed-off-by: Heng Qian <qianheng@amazon.com>

Support Aggregate Push Down

b9bb880

Signed-off-by: Heng Qian <qianheng@amazon.com>

Rebase and resolve conflict

52ff868

Signed-off-by: Heng Qian <qianheng@amazon.com>

qianheng-aws requested review from GumpacG, LantaoJin, MaxKsyunz, Swiddis, YANG-DB, Yury-Fridlyand, acarbonetto, anirudha, dai-chen, derek-ho, forestmvey, joshuali925, kavithacm, mengweieric, noCharger, penghuo, ps48, seankao-az and ykmr1224 as code owners March 6, 2025 10:39

Add TODO

6d8465d

Signed-off-by: Heng Qian <qianheng@amazon.com>

LantaoJin reviewed Mar 7, 2025

View reviewed changes

penghuo reviewed Mar 7, 2025

View reviewed changes

penghuo added the calcite calcite migration releated label Mar 7, 2025

qianheng-aws added 2 commits March 7, 2025 13:49

Address comments

7c32bb8

Signed-off-by: Heng Qian <qianheng@amazon.com>

Merge remote-tracking branch 'refs/remotes/origin/feature/calcite-eng…

e750c04

…ine' into feature/calcite-engine-pushdown-agg

Merge remote-tracking branch 'refs/remotes/origin/feature/calcite-eng…

4f418ff

…ine' into feature/calcite-engine-pushdown-agg

LantaoJin approved these changes Mar 7, 2025

View reviewed changes

LantaoJin merged commit dcf2057 into opensearch-project:feature/calcite-engine Mar 7, 2025
4 of 13 checks passed

LantaoJin mentioned this pull request Apr 2, 2025

[GitHub Request] Add qianheng-aws as maintainer of OpenSearch SQL opensearch-project/.github#313

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calcite enable pushdown aggregation#3389

Calcite enable pushdown aggregation#3389
LantaoJin merged 7 commits intoopensearch-project:feature/calcite-enginefrom
qianheng-aws:feature/calcite-engine-pushdown-agg

qianheng-aws commented Mar 6, 2025

Uh oh!

LantaoJin commented Mar 7, 2025

Uh oh!

LantaoJin left a comment

Uh oh!

LantaoJin Mar 7, 2025

Uh oh!

LantaoJin Mar 7, 2025

Uh oh!

LantaoJin Mar 7, 2025

Uh oh!

penghuo Mar 7, 2025

Uh oh!

qianheng-aws Mar 7, 2025

Uh oh!

penghuo Mar 7, 2025

Uh oh!

qianheng-aws Mar 7, 2025 •

edited

Loading

Uh oh!

qianheng-aws commented Mar 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

qianheng-aws commented Mar 6, 2025

Description

Related Issues

Check List

Uh oh!

LantaoJin commented Mar 7, 2025

Uh oh!

LantaoJin left a comment

Choose a reason for hiding this comment

Uh oh!

LantaoJin Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

LantaoJin Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

LantaoJin Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

penghuo Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

qianheng-aws Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

penghuo Mar 7, 2025

Choose a reason for hiding this comment

Uh oh!

qianheng-aws Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qianheng-aws commented Mar 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

qianheng-aws Mar 7, 2025 •

edited

Loading