fix: Improve the externalwindow documentation while fixing the bug in the group_concat function - #35078
Conversation
![gemini-code-assist[bot]](https://avatars.github.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the EXTERNAL_WINDOW feature, which allows window time ranges to be explicitly defined by subqueries for complex analysis. The changes include comprehensive documentation updates, implementation of scalar expression evaluation in the executor for both external and interval windows, and a fix for group_concat where separators were previously dropped during merge-aligned operations. Additionally, extensive regression tests have been added for group_concat, leastsquares, and external window aggregate functions. The review feedback highlights the need to explicitly set *dataLen to zero when handling NULL inputs in gconcatHelper and to update the lino variable for accurate error reporting in the executor code.
There was a problem hiding this comment.
Pull request overview
This PR fixes GROUP_CONCAT behavior in merge-aligned execution paths (e.g., supertable INTERVAL and EXTERNAL_WINDOW) by ensuring scalar expressions are evaluated during merge, hardens GROUP_CONCAT against NULL inputs, and expands external window documentation and regression test coverage.
Changes:
- Execute scalar expressions in merge-aligned interval/external-window operators to preserve
GROUP_CONCAT(..., separator)semantics. - Add NULL guard + refresh
hasNcharhandling in theGROUP_CONCATbuiltin implementation. - Add/extend regression tests and update docs/BNF to include
EXTERNAL_WINDOW.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
source/libs/function/src/builtinsimpl.c |
Adds NULL guard in gconcatHelper and refreshes hasNchar from detected inputs to avoid charset conversion issues. |
source/libs/executor/src/timewindowoperator.c |
Applies scalar expression evaluation in merge-aligned interval aggregation and initializes scalar expr support from plan node. |
source/libs/executor/src/externalwindowoperator.c |
Applies scalar expression evaluation in merge-aligned external window aggregation and initializes scalar expr support from plan node. |
test/cases/11-Functions/02-Aggregate/test_agg_gconcat.py |
Adds regression tests for merge-aligned separator handling plus additional GROUP_CONCAT scenarios (NULLs, multi-col, separator variants, etc.). |
test/cases/11-Functions/02-Aggregate/test_fun_agg_leastsquares.py |
Adds crash-guard + determinism tests for LEASTSQUARES on supertables with duplicate timestamps across children. |
test/cases/13-TimeSeriesExt/08-ExternalWindow/test_external.py |
Extends external-window test matrix to cover additional aggregate functions including GROUP_CONCAT. |
docs/zh/05-basic/03-query.md |
Adds EXTERNAL_WINDOW to the window clause overview and references the detailed doc. |
docs/zh/14-reference/03-taos-sql/20-select.md |
Updates window clause BNF and adds an EXTERNAL_WINDOW description entry. |
docs/zh/14-reference/03-taos-sql/24-distinguished.md |
Adds EXTERNAL_WINDOW to BNF and provides a detailed “外部窗口” section with rules and examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
81f9a8b to
acd098e
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…al_window docs/tests - executor: init scalarSupp and call projectApplyFunctions in MergeAlignedExternalWindowOperator and TimeMergeAlignedIntervalAggOperator so COLUMN-type separator parameters are materialized correctly - function: add NULL guard in gconcatHelper to prevent SIGSEGV on NULL input; refresh hasNchar flag after NCHAR detection to avoid doConvertUCS4 error - docs: add EXTERNAL_WINDOW to window_clause BNF and reference in 03-query.md, 20-select.md, 24-distinguished.md - test: add 6 new GROUP_CONCAT test methods in test_agg_gconcat.py (separator_merge_aligned, null_handling, multi_column, nchar_varchar_mix, separator_variants, group_by, edge_cases) - test: add test_leastsquares_dup_ts for LEASTSQUARES on supertable with duplicate timestamps across child tables - test: fix test_external.py LEASTSQUARES expectation (now succeeds, not errors) on supertable external window queries
7a0b5b3 to
a181d79
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
facetosea
changed the title
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… the group_concat function (#35078)
feici02
added
keep branch
external_window docs/tests
function: add NULL guard in gconcatHelper to prevent SIGSEGV on NULL input; refresh hasNchar flag after NCHAR detection to avoid doConvertUCS4 error
docs: Improve the external window documentation, in 03-query.md, 20-select.md, 24-distinguished.md
test: add 6 new GROUP_CONCAT test methods in test_agg_gconcat.py (separator_merge_aligned, null_handling, multi_column, nchar_varchar_mix, separator_variants, group_by, edge_cases)
test: add test_leastsquares_dup_ts for LEASTSQUARES on supertable with duplicate timestamps across child tables
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.