-
Notifications
You must be signed in to change notification settings - Fork 5k
fix: state window handle all-null datablock error #33439
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
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 resolves a critical issue in the state window aggregation mechanism, specifically addressing errors that occurred when processing data blocks containing entirely null values or when state windows extended across multiple data blocks. The changes involve significant refactoring of the aggregation logic, introducing more robust handling for partial data blocks and nulls, and ensuring that state windows are correctly processed, initialized, and reset. This enhances the reliability and accuracy of state window operations, especially in complex data streaming scenarios. 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 review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request addresses an error in state window handling when a datablock is entirely composed of null values. The fix involves a significant refactoring of the state window aggregation logic. Now, it buffers unprocessed parts of a data block and merges them with subsequent blocks, which correctly handles state windows that span across multiple blocks, especially in the presence of nulls. The logic has also been improved by extracting helper functions like processClosedStateWindow, which enhances code clarity and maintainability. My review includes a critical comment to remove a leftover debug print statement and a couple of medium-severity suggestions for code and repository hygiene.
| SExprSupp* pSup = &pOperator->exprSupp; | ||
|
|
||
| SColumnInfoData* pStateColInfoData = taosArrayGet(pBlock->pDataBlock, pInfo->stateCol.slotId); | ||
| printDataBlock(pBlock, "tooony", "tooony", 9898); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
ebc2058 to
6c742eb
Compare
|
/gemini review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the state window aggregation logic to correctly handle data blocks that span window boundaries, particularly addressing an issue with all-null data blocks. The changes introduce a mechanism to carry over partially processed data blocks (pUnfinishedBlock) to subsequent processing loops, ensuring that state windows are correctly demarcated and aggregated even when they cross block boundaries.
Key changes include:
- Extracting a
processClosedStateWindowfunction to encapsulate window processing logic, improving code reuse and readability. - Modifying
doStateWindowAggImplto process slices of a data block, enabling partial processing. - Updating
openStateWindowAggOptrto manage block merging and carry-over of unfinished blocks. - Adding a
resetWindowRowsSuphelper function to centralize state reset logic.
The overall approach is sound and addresses the described problem effectively. I have two suggestions: one critical fix to prevent a potential crash with empty data blocks, and one for improving performance and code clarity by moving a redundant check out of a loop.
| bool blockEndsWithNullStateCol = colDataIsNull(pStateColInfoData, | ||
| pBlock->info.rows, *endIndex - 1, pAgg); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
b56dd19 to
00e2517
Compare
Description
Please briefly describe the code changes in this pull request.
Checklist
Please check the items in the checklist if applicable.