fix: state window handles partititon by error#33782
Conversation
Tony2h
requested review from
a team,
dapan1121,
guanshengliang and
hzcheng
as code owners
Summary of ChangesHello @Tony2h, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses and fixes an error in how state window functions handle data partitioning, particularly concerning the accurate definition of window boundaries and the processing of null values across different data blocks and groups. The changes ensure that state windows are correctly formed and aggregated, even when data contains nulls or spans multiple partitions, leading to more reliable query results. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a bug in how state_window handles partition by clauses. The changes correctly manage group boundaries within data blocks by processing the unclosed window of the previous group before starting a new one. Key fixes include updating the group ID of merged blocks, correctly initializing timestamps, and adjusting window start/end key calculations. The addition of a new test case for this scenario is a great improvement. I've found one potential bug related to a type mismatch and have a suggestion to improve code readability.
| pTaskInfo->code = TSDB_CODE_QRY_EXECUTOR_INTERNAL_ERROR; | ||
| T_LONG_JMP(pTaskInfo->env, TSDB_CODE_QRY_EXECUTOR_INTERNAL_ERROR); | ||
| } | ||
| int64_t gid = pBlock->info.id.groupId; |
There was a problem hiding this comment.
The variable gid is declared as int64_t, but it is assigned from pBlock->info.id.groupId, which is of type uint64_t. This can lead to incorrect behavior for large group IDs due to potential overflow or incorrect sign interpretation. It should be declared as uint64_t to match the type of pBlock->info.id.groupId and pRowSup->groupId.
uint64_t gid = pBlock->info.id.groupId;| pRowSup->startRowIndex = pRowSup->groupId = 0; | ||
| pRowSup->numOfRows = pRowSup->numNullRows = 0; |
There was a problem hiding this comment.
For better readability and to avoid potential subtle issues, consider splitting these chained assignments into separate lines.
pRowSup->startRowIndex = 0;
pRowSup->groupId = 0;
pRowSup->numOfRows = 0;
pRowSup->numNullRows = 0;There was a problem hiding this comment.
Pull request overview
This PR fixes an error in state window handling when used with PARTITION BY clauses, specifically addressing issues where partition boundaries and null values in data blocks were not being handled correctly.
Key Changes
- Added group change detection and handling logic to properly close state windows when partition groups change
- Fixed null row tracking to use timestamps instead of row indices for correct end window calculation
- Enhanced window initialization logic to correctly determine start keys when handling null rows across partition boundaries
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| test/cases/13-TimeSeriesExt/04-StateWindow/test_state_window_null_block.py | Added comprehensive test case check_partition_and_null() to verify state window behavior with partition by and null values |
| test/cases/05-VirtualTables/in/test_vstable_select_test_partition.in | Updated test query from subquery approach to direct partition by with state_window, adding partition column to output |
| test/cases/05-VirtualTables/ans/test_vstable_select_test_partition.ans | Updated expected results to match new query format with partition column included |
| test/cases/05-VirtualTables/ans/test_vctable_select_test_partition.ans | Updated expected results for child table variant to match new query format |
| source/libs/executor/src/timewindowoperator.c | Core fix: Added group change detection, fixed null row timestamp tracking, improved window boundary calculations, and added empty window guard |
| source/libs/executor/inc/executorInt.h | Updated prevTs initialization to INT64_MIN and improved reset logic for window state tracking |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Description
Please briefly describe the code changes in this pull request.
Jira: https://jira.taosdata.com:18080/browse/TD-
Checklist
Please check the items in the checklist if applicable.