Skip to content

Conversation

discord9
Copy link
Contributor

@discord9 discord9 commented Sep 23, 2025

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

fix a bug where group by exprs in upper aggr in step aggr failed to change to column refs, causing
step aggr to silently failed and cause wrong query plan(sliently!), fix:

  • fail explicitly when transformer failed
  • upper group by expr use column ref instead of original exprs
  • also correctly deduce aggregate's original return type

PR Checklist

Please convert it to a draft if some of the following conditions are not met.

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR requires documentation updates.
  • API changes are backward compatible.
  • Schema or data changes are backward compatible.

@discord9 discord9 requested review from evenyag and a team as code owners September 23, 2025 07:40
@github-actions github-actions bot added size/M docs-not-required This change does not impact docs. labels Sep 23, 2025
@discord9 discord9 requested review from waynexia and WenyXu September 23, 2025 07:47
@killme2008 killme2008 requested a review from Copilot September 23, 2025 07:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug in step aggregation where GROUP BY expressions weren't properly converted to column references in upper aggregation nodes. The fix ensures that step aggregation fails explicitly when transformation fails and updates GROUP BY expressions to use column references instead of original expressions.

Key changes:

  • Updated step aggregation transformation to use column references for GROUP BY expressions in upper nodes
  • Modified error handling to fail explicitly when transformation fails instead of silently continuing
  • Added more test data to validate the fix with additional query patterns

Reviewed Changes

Copilot reviewed 7 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/common/function/src/aggrs/aggr_wrapper.rs Fixes GROUP BY expressions to use column references in upper aggregation nodes
src/query/src/dist_plan/commutativity.rs Updates transformer error handling to fail explicitly and propagate errors
src/query/src/dist_plan/analyzer.rs Updates function signatures and error handling for transformer results
tests/cases/distributed/explain/step_aggr_basic.sql Adds test data with more diverse host patterns
tests/cases/distributed/explain/step_aggr_basic.result Updates expected results reflecting the test data changes
tests/cases/distributed/explain/step_aggr.sql Adds new test cases for step aggregation validation
tests/cases/distributed/explain/step_aggr.result Updates expected results for new test cases

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@WenyXu WenyXu mentioned this pull request Sep 23, 2025
13 tasks
@discord9 discord9 force-pushed the fix/failed_aggr_should_fail_explict branch from 5df4c7d to 84e3a7c Compare September 23, 2025 13:01
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

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

LGTM

@evenyag
Copy link
Contributor

evenyag commented Sep 24, 2025

fix a bug where group by exprs in upper aggr in step aggr failed to change to column refs, causing
step aggr to silently failed and cause wrong query plan(sliently!)

What's the wrong query plan generated without this fix?

@discord9
Copy link
Contributor Author

fix a bug where group by exprs in upper aggr in step aggr failed to change to column refs, causing
step aggr to silently failed and cause wrong query plan(sliently!)

What's the wrong query plan generated without this fix?

i.e. for a group by date_bin('1s', ts) the upper plan (without this fix) will not change the group exprs,
but in fact it should use col("date_bin('1s', ts)") which is a column ref to the lower plan's output

Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: discord9 <[email protected]>
@killme2008 killme2008 added this pull request to the merge queue Sep 24, 2025
Merged via the queue into GreptimeTeam:main with commit 238ed00 Sep 24, 2025
42 checks passed
WenyXu pushed a commit to WenyXu/greptimedb that referenced this pull request Sep 25, 2025
* fix: group by expr not as column

Signed-off-by: discord9 <[email protected]>

* test: dist analyzer date_bin

Signed-off-by: discord9 <[email protected]>

* ???fix wip

Signed-off-by: discord9 <[email protected]>

* fix: deduce using correct input fields

Signed-off-by: discord9 <[email protected]>

* refactor: clearer wrapper

Signed-off-by: discord9 <[email protected]>

* chore: update sqlness

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: rm todo

Signed-off-by: discord9 <[email protected]>

---------

Signed-off-by: discord9 <[email protected]>
WenyXu pushed a commit to WenyXu/greptimedb that referenced this pull request Sep 25, 2025
* fix: group by expr not as column

Signed-off-by: discord9 <[email protected]>

* test: dist analyzer date_bin

Signed-off-by: discord9 <[email protected]>

* ???fix wip

Signed-off-by: discord9 <[email protected]>

* fix: deduce using correct input fields

Signed-off-by: discord9 <[email protected]>

* refactor: clearer wrapper

Signed-off-by: discord9 <[email protected]>

* chore: update sqlness

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: rm todo

Signed-off-by: discord9 <[email protected]>

---------

Signed-off-by: discord9 <[email protected]>
discord9 added a commit to WenyXu/greptimedb that referenced this pull request Sep 28, 2025
* fix: group by expr not as column

Signed-off-by: discord9 <[email protected]>

* test: dist analyzer date_bin

Signed-off-by: discord9 <[email protected]>

* ???fix wip

Signed-off-by: discord9 <[email protected]>

* fix: deduce using correct input fields

Signed-off-by: discord9 <[email protected]>

* refactor: clearer wrapper

Signed-off-by: discord9 <[email protected]>

* chore: update sqlness

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: rm todo

Signed-off-by: discord9 <[email protected]>

---------

Signed-off-by: discord9 <[email protected]>
discord9 added a commit that referenced this pull request Sep 28, 2025
* fix: print the output message of the error in admin fn macro (#6994)

Signed-off-by: evenyag <[email protected]>
Signed-off-by: discord9 <[email protected]>

* fix: make EXPIRE (keyword) parsing case-insensitive, when creating flow (#6997)

fix: make EXPIRE keyword case-insensitive in CREATE FLOW parser

Signed-off-by: Shyamnatesan <[email protected]>
Signed-off-by: discord9 <[email protected]>

* fix: promql range function has incorrect timestamps (#7006)

* fix: promql range function has incorrect timestamps

Signed-off-by: Ruihang Xia <[email protected]>

* simplify

Signed-off-by: Ruihang Xia <[email protected]>

---------

Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: discord9 <[email protected]>

* fix: incorrect timestamp resolution in information_schema.partitions table (#7004)

* fix: incorrect timestamp resolution in information_schema.partitions table

Signed-off-by: Ruihang Xia <[email protected]>

* use second for all fields in partitions table

Signed-off-by: Ruihang Xia <[email protected]>

* update sqlness result

Signed-off-by: Ruihang Xia <[email protected]>

---------

Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: discord9 <[email protected]>

* fix: match promql column reference in case sensitive way (#7013)

Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: discord9 <[email protected]>

* fix: group by expr not as column in step aggr (#7008)

* fix: group by expr not as column

Signed-off-by: discord9 <[email protected]>

* test: dist analyzer date_bin

Signed-off-by: discord9 <[email protected]>

* ???fix wip

Signed-off-by: discord9 <[email protected]>

* fix: deduce using correct input fields

Signed-off-by: discord9 <[email protected]>

* refactor: clearer wrapper

Signed-off-by: discord9 <[email protected]>

* chore: update sqlness

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* chore: rm todo

Signed-off-by: discord9 <[email protected]>

---------

Signed-off-by: discord9 <[email protected]>

* fix: skip placeholder when partition columns (#7020)

Signed-off-by: discord9 <[email protected]>

* chore: add function for getting started on metasrv (#7022)

Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: discord9 <[email protected]>

* fix: not step when aggr have order by/filter (#7015)

* fix: not applied

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* test: confirm order by not push down

Signed-off-by: discord9 <[email protected]>

---------

Signed-off-by: discord9 <[email protected]>

* feat: supports expression in TQL params (#7014)

* feat: supports expression in TQL params

Signed-off-by: Dennis Zhuang <[email protected]>

* chore: by cr comments

Signed-off-by: Dennis Zhuang <[email protected]>

* chore: comment

Signed-off-by: Dennis Zhuang <[email protected]>

* chore: by cr comments

Signed-off-by: Dennis Zhuang <[email protected]>

---------

Signed-off-by: Dennis Zhuang <[email protected]>
Signed-off-by: discord9 <[email protected]>

* feat: update dashboard to v0.11.6 (#7026)

Co-authored-by: ZonaHex <[email protected]>
Signed-off-by: discord9 <[email protected]>

* fix: step aggr merge phase not order nor filter (#6998)

* fix: not order

Signed-off-by: discord9 <[email protected]>

* test: redacted

Signed-off-by: discord9 <[email protected]>

* feat: fix up state wrapper

Signed-off-by: discord9 <[email protected]>

* df last_value state not as promised!

Signed-off-by: discord9 <[email protected]>

* fix?: could fix better

Signed-off-by: discord9 <[email protected]>

* test: unstable result

Signed-off-by: discord9 <[email protected]>

* fix: work around by fixing state

Signed-off-by: discord9 <[email protected]>

* chore: after rebase fix

Signed-off-by: discord9 <[email protected]>

* chore: finish some todo

Signed-off-by: discord9 <[email protected]>

* chore: per copilot

Signed-off-by: discord9 <[email protected]>

* refactor: not fix but just notify mismatch

Signed-off-by: discord9 <[email protected]>

* chore: warn -> debug state mismatch

Signed-off-by: discord9 <[email protected]>

* chore: refine error msg

Signed-off-by: discord9 <[email protected]>

* test: sqlness add last_value date_bin test

Signed-off-by: discord9 <[email protected]>

* ?: substrait order by decode failure

Signed-off-by: discord9 <[email protected]>

* unit test reproduce that

Signed-off-by: discord9 <[email protected]>

* feat: support state wrapper's order serde in substrait

Signed-off-by: discord9 <[email protected]>

* refactor: stuff

Signed-off-by: discord9 <[email protected]>

* test: standalone/distributed different exec

Signed-off-by: discord9 <[email protected]>

* fmt

Signed-off-by: discord9 <[email protected]>

* chore: per review

Signed-off-by: discord9 <[email protected]>

* refactor: closure

Signed-off-by: discord9 <[email protected]>

* test: first value order by

Signed-off-by: discord9 <[email protected]>

* refactor: per cr

Signed-off-by: discord9 <[email protected]>

* feat: ScanHint last_value last row selector

Signed-off-by: discord9 <[email protected]>

* docs: per cr

Signed-off-by: discord9 <[email protected]>

---------

Signed-off-by: discord9 <[email protected]>

* chore: bump version to 0.17.2

Signed-off-by: WenyXu <[email protected]>
Signed-off-by: discord9 <[email protected]>

* chore: not warning

Signed-off-by: discord9 <[email protected]>

---------

Signed-off-by: evenyag <[email protected]>
Signed-off-by: discord9 <[email protected]>
Signed-off-by: Shyamnatesan <[email protected]>
Signed-off-by: Ruihang Xia <[email protected]>
Signed-off-by: shuiyisong <[email protected]>
Signed-off-by: Dennis Zhuang <[email protected]>
Signed-off-by: WenyXu <[email protected]>
Co-authored-by: Yingwen <[email protected]>
Co-authored-by: shyam <[email protected]>
Co-authored-by: Ruihang Xia <[email protected]>
Co-authored-by: discord9 <[email protected]>
Co-authored-by: shuiyisong <[email protected]>
Co-authored-by: dennis zhuang <[email protected]>
Co-authored-by: ZonaHe <[email protected]>
Co-authored-by: ZonaHex <[email protected]>
Co-authored-by: discord9 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs-not-required This change does not impact docs. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants