Skip to content

Conversation

@yuancu
Copy link
Collaborator

@yuancu yuancu commented Aug 27, 2025

Description

This PR allows makes the query source=my-index | stats count() by span(1h) equivalent to source=my-index | stats count() by span(@timestamp, 1h)

  • Update documentations to reflect this change
  • Resolve by span(1d) as by span(@timestamp, 1d)
  • Throw exception for zero span

Related Issues

Resolves #4136, resolves #4527

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • 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.

@yuancu yuancu self-assigned this Aug 27, 2025
@yuancu yuancu added the enhancement New feature or request label Aug 27, 2025
LantaoJin
LantaoJin previously approved these changes Aug 29, 2025
Copy link
Member

@LantaoJin LantaoJin left a comment

Choose a reason for hiding this comment

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

@yuancu can you fix the conflicts

public RexNode visitSpan(Span node, CalcitePlanContext context) {
RexNode field = analyze(node.getField(), context);
RexNode field;
if (node.getField() != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will it be better to set field in span to be new Field(new QualifiedName(OpenSearchConstants.IMPLICIT_FIELD_TIMESTAMP)) if field is null? Then we don't need any change here.

And I'm wondering if v2 works well since we only make change for calcite visitor.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I referred it here because I thought the AST level should be consistent with users' input. E.g. BY span(1h) should be parsed to Span(value=1, unit=hour, field=null). Subsequent implicit reference can be added later because this semantic of how absent field should be interpreted is in a higher level than syntax tree construction.

But it's also not bizarre to refer it while building AST tree. I'll modify the implementation accordingly.

- Additionally refactored visitTimechartCommand to reuse spanLiteral definition

Signed-off-by: Yuanchun Shen <[email protected]>
if (field != null) {
return StringUtils.format("span(%s, %s %s)", field, value, node.getUnit().getName());
} else {
return StringUtils.format("span(%s %s)", value, node.getUnit().getName());
Copy link
Member

Choose a reason for hiding this comment

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

no test for verification?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Reverted this change to Span as span will always come with a field with latest implementation -- I inject @timestamp to spans without a field specified in the AST layer now.

qianheng-aws
qianheng-aws previously approved these changes Sep 24, 2025
@opensearch-trigger-bot
Copy link
Contributor

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

@RyanL1997
Copy link
Collaborator

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

Hi @yuancu , is this implementation still in track?

LantaoJin
LantaoJin previously approved these changes Oct 15, 2025
Signed-off-by: Yuanchun Shen <[email protected]>
@yuancu yuancu added the bug Something isn't working label Oct 17, 2025
@qianheng-aws qianheng-aws merged commit c30d5d0 into opensearch-project:main Oct 21, 2025
36 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 21, 2025
* Support refering to implicit @timestamp field in time-based aggregations

Signed-off-by: Yuanchun Shen <[email protected]>

* Update documentation of stats to reflect that span can be used without specifying a field

Signed-off-by: Yuanchun Shen <[email protected]>

* Move @timestamp reference to AST layer

- Additionally refactored visitTimechartCommand to reuse spanLiteral definition

Signed-off-by: Yuanchun Shen <[email protected]>

* Unit test visitSpanLiteral, vistSpanClause, and visitTimechartParamter

Signed-off-by: Yuanchun Shen <[email protected]>

* Revert changes to Span will always have a field with the current implementation

Signed-off-by: Yuanchun Shen <[email protected]>

* Throw exception for zero span

Signed-off-by: Yuanchun Shen <[email protected]>

---------

Signed-off-by: Yuanchun Shen <[email protected]>
(cherry picked from commit c30d5d0)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@yuancu yuancu deleted the issues/4136 branch October 21, 2025 06:06
qianheng-aws pushed a commit that referenced this pull request Oct 21, 2025
…n span (#4610)

* Support refering to implicit `@timestamp` field in span (#4138)

* Support refering to implicit @timestamp field in time-based aggregations

Signed-off-by: Yuanchun Shen <[email protected]>

* Update documentation of stats to reflect that span can be used without specifying a field

Signed-off-by: Yuanchun Shen <[email protected]>

* Move @timestamp reference to AST layer

- Additionally refactored visitTimechartCommand to reuse spanLiteral definition

Signed-off-by: Yuanchun Shen <[email protected]>

* Unit test visitSpanLiteral, vistSpanClause, and visitTimechartParamter

Signed-off-by: Yuanchun Shen <[email protected]>

* Revert changes to Span will always have a field with the current implementation

Signed-off-by: Yuanchun Shen <[email protected]>

* Throw exception for zero span

Signed-off-by: Yuanchun Shen <[email protected]>

---------

Signed-off-by: Yuanchun Shen <[email protected]>
(cherry picked from commit c30d5d0)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Change pattern variable to cast

Signed-off-by: Yuanchun Shen <[email protected]>

---------

Signed-off-by: Yuanchun Shen <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Yuanchun Shen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 2.19-dev bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] timechart command with zero span throws execution exception [FEATURE] Support implicit @timestamp field access in span

4 participants