Skip to content

Support nested aggregation when calcite enabled#3696

Closed
LantaoJin wants to merge 2 commits intoopensearch-project:mainfrom
LantaoJin:pr/issues/2739
Closed

Support nested aggregation when calcite enabled#3696
LantaoJin wants to merge 2 commits intoopensearch-project:mainfrom
LantaoJin:pr/issues/2739

Conversation

@LantaoJin
Copy link
Copy Markdown
Member

@LantaoJin LantaoJin commented May 29, 2025 •
edited
Loading

Description

Support nested aggregation in PPL only when calcite enabled.

With this PR, follow PPL query is able to execute a nested aggregation query.

source=logs | where response = 200 | stats min(pages.load_time)

And it equals the DSL

GET logs/_search
{
  "query": {
    "match": { "response": "200" }
  },
  "aggs": {
    "pages": {
      "nested": {
        "path": "pages"
      },
      "aggs": {
        "min_load_time": { "min": { "field": "pages.load_time" } }
      }
    }
  }
}

Deprecated implementation for SQL: #2814

Limitation:

  • PPL only
  • Calcite should be enabled
  • Aggregate pushdown happens

Related Issues

PR partially resolves #2813 and #2529, and #2739

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.

@LantaoJin
Support nested aggregation when calcite enabled
b56d813 
Signed-off-by: Lantao Jin <ltjin@amazon.com>
@LantaoJin
Copy link
Copy Markdown
Member Author

LantaoJin commented May 29, 2025

CI failure is not related to this PR. It caused by opensearch-project/OpenSearch#18124

@LantaoJin LantaoJin marked this pull request as ready for review May 29, 2025 11:17
dai-chen
dai-chen reviewed May 30, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@dai-chen dai-chen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QQ: I recall we didn't support nested field type in V2 engine, right? Did we add it to Calcite or this is only for pushdown? Ref: #52

@LantaoJin
Copy link
Copy Markdown
Member Author

LantaoJin commented May 30, 2025 •
edited
Loading

QQ: I recall we didn't support nested field type in V2 engine, right? Did we add it to Calcite or this is only for pushdown? Ref: #52

Nested field maps to ARRAY type in OpenSearch Core (you can get it by creating an index with nested field and checking its mapping via DSL), so IMO v2 supports nested field with the ExprCoreType.ARRAY type.

I believe we also can support nested aggregation in v2 but the fixing will be complex by my trying. Now I have no idea how to pass the fieldTypes map to AggregatorBuilder logic. We need it to identify whether or not the root in aggregation NamedExpression root.subfield is a ARRAY type.

For pushdown disabled, I think we need additional effort to correct the aggregation behavior. So I add a TODO in code. Or find a way to throw an exception.

@qianheng-aws
Copy link
Copy Markdown
Collaborator

qianheng-aws commented May 30, 2025

The value of nested field in PPL is incorrect.

Seems we will only show the first element of that nested fields while it's actually an inner field of an array of object. That's why it will calculate wrong results if no push down.

sql/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java

Line 225 in df9f5dd

// For array types only first index currently supported.

DSL:

POST /test1/_search
{
  "_source":{"includes":["address.area"]}
}

{
  "took": 11,
  "timed_out": false,
  "_shards": {
    "total": 1,
    "successful": 1,
    "skipped": 0,
    "failed": 0
  },
  "hits": {
    "total": {
      "value": 1,
      "relation": "eq"
    },
    "max_score": 1.0,
    "hits": [
      {
        "_index": "test1",
        "_id": "u6OQIJcBpwe8-qLMf_ZG",
        "_score": 1.0,
        "_source": {
          "address": [
            {
              "area": 300.13
            },
            {
              "area": 400.99
            },
            {
              "area": 127.4
            },
            {
              "area": 10.24
            }
          ]
        }
      }
    ]
  }
}

PPL:

POST /_plugins/_ppl
source = test1 | fields address.area

{
  "schema": [
    {
      "name": "address.area",
      "type": "double"
    }
  ],
  "datarows": [
    [
      300.13
    ]
  ],
  "total": 1,
  "size": 1
}

@LantaoJin
Copy link
Copy Markdown
Member Author

LantaoJin commented Jun 4, 2025 •
edited
Loading

Seems we will only show the first element of that nested fields while it's actually an inner field of an array of object. That's why it will calculate wrong results if no push down.

sql/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java

Line 225 in df9f5dd

// For array types only first index currently supported.

Thanks, please let me know why we just return first element? Let me try to change this logic first.

@LantaoJin
Copy link
Copy Markdown
Member Author

LantaoJin commented Jun 4, 2025 •
edited
Loading

Seems we will only show the first element of that nested fields while it's actually an inner field of an array of object. That's why it will calculate wrong results if no push down.

sql/core/src/main/java/org/opensearch/sql/data/model/ExprValueUtils.java

Line 225 in df9f5dd

// For array types only first index currently supported.

Thanks, please let me know why we just return first element? Let me try to change this logic first.

Perhaps I know the reason now. I update above code to

wholePathValue = new ExprCollectionValue(value.collectionValue().stream().map(e -> e.keyValue(paths.getFirst())).toList());

but it doesn't work since we flatten the schema (mapping):

sql/opensearch/src/main/java/org/opensearch/sql/opensearch/data/type/OpenSearchDataType.java

Lines 294 to 296 in 5eb2e5b

var nextPrefix =
prefix.isEmpty() ? entryKey : String.format("%s.%s", prefix, entryKey);
result.put(nextPrefix, entry.getValue().cloneEmpty());

For example address.area is mapping to ExprCoreType.Double and we don't have an Array[Double] type in ExprCoreType. To support reading a sub-field, we need support Array type of ExprCoreType.

@LantaoJin LantaoJin mentioned this pull request Jun 27, 2025
Open
@opensearch-trigger-bot
Copy link
Copy Markdown
Contributor

opensearch-trigger-bot bot commented Jul 4, 2025

This PR is stalled because it has been open for 30 days with no activity.

@Swiddis
Copy link
Copy Markdown
Collaborator

Swiddis commented Nov 25, 2025

@LantaoJin what's the status of this?

@opensearch-trigger-bot
Copy link
Copy Markdown
Contributor

opensearch-trigger-bot bot commented Dec 10, 2025

This PR is stalled because it has been open for 2 weeks with no activity.

@LantaoJin LantaoJin mentioned this pull request Dec 12, 2025
Closed
@LantaoJin
Copy link
Copy Markdown
Member Author

LantaoJin commented Dec 12, 2025

@LantaoJin what's the status of this?

Deprioritized it since we created a META ticket #4625 to discuss all cases for nested type as there is no user cases for customer F. I hope we can have a roadmap instead of keep the nested type as a Schrödinger's cat.

This was referenced Dec 18, 2025
Merged
Merged
@LantaoJin
Copy link
Copy Markdown
Member Author

LantaoJin commented Dec 19, 2025

Close this as the refactor #4979 opened.

@LantaoJin LantaoJin closed this Dec 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dai-chen dai-chen dai-chen left review comments

@ps48 ps48 Awaiting requested review from ps48 ps48 is a code owner

@kavithacm kavithacm Awaiting requested review from kavithacm

@derek-ho derek-ho Awaiting requested review from derek-ho

@joshuali925 joshuali925 Awaiting requested review from joshuali925 joshuali925 is a code owner

@YANG-DB YANG-DB Awaiting requested review from YANG-DB

@mengweieric mengweieric Awaiting requested review from mengweieric mengweieric is a code owner

@Swiddis Swiddis Awaiting requested review from Swiddis Swiddis is a code owner

@penghuo penghuo Awaiting requested review from penghuo penghuo is a code owner

@seankao-az seankao-az Awaiting requested review from seankao-az

@MaxKsyunz MaxKsyunz Awaiting requested review from MaxKsyunz

@Yury-Fridlyand Yury-Fridlyand Awaiting requested review from Yury-Fridlyand

@anirudha anirudha Awaiting requested review from anirudha anirudha is a code owner

@forestmvey forestmvey Awaiting requested review from forestmvey

@acarbonetto acarbonetto Awaiting requested review from acarbonetto acarbonetto is a code owner

@GumpacG GumpacG Awaiting requested review from GumpacG

@ykmr1224 ykmr1224 Awaiting requested review from ykmr1224 ykmr1224 is a code owner

@noCharger noCharger Awaiting requested review from noCharger noCharger is a code owner

@qianheng-aws qianheng-aws Awaiting requested review from qianheng-aws qianheng-aws is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[FEATURE] Support nested aggregation

4 participants

@LantaoJin @qianheng-aws @Swiddis @dai-chen