merge: merge main to 3.0 #33839
merge: merge main to 3.0 #33839
Conversation
Closes: 6492139991
* fix: memleak * fix: use escaped json for show create sql
* feat: add taos_connect_with_dsn func * docs: add taos_connect_with_dsn doc
…insert/opentsdb_telnet_protocol_taosc_insert.py:986 (#33785)
Tony2h
requested review from
a team,
dapan1121,
feici02,
guanshengliang,
hzcheng and
zitsen
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 is a merge from the 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 is a merge from main to the 3.0 branch, incorporating a wide variety of changes. These include documentation updates and typo fixes, bug fixes in the query execution engine, and some refactoring.
My review has focused on the C code changes. I've identified a couple of areas for improvement:
- An inefficiency in schema modification handling where hash tables are rebuilt repeatedly within a loop.
- A potential correctness issue in the
state_windowoperator due to the removal of a duplicate timestamp check.
Overall, the changes seem to be moving in the right direction, especially the fixes for state_window with PARTITION BY and the refactoring of the transport cache. Please see my detailed comments for specific suggestions.
| pInfo->tsSlotId); | ||
| if (NULL == pColInfoData) { | ||
| pTaskInfo->code = terrno; | ||
| T_LONG_JMP(pTaskInfo->env, terrno); | ||
| } | ||
| TSKEY* tsList = (TSKEY*)pColInfoData->pData; | ||
|
|
||
| struct SColumnDataAgg* pAgg = NULL; | ||
| struct SColumnDataAgg* pAgg = (pBlock->pBlockAgg != NULL) ? | ||
| &pBlock->pBlockAgg[pInfo->stateCol.slotId] : | ||
| NULL; | ||
| EStateWinExtendOption extendOption = pInfo->extendOption; | ||
| SWindowRowsSup* pRowSup = &pInfo->winSup; | ||
|
|
||
| if (pRowSup->groupId != gid) { | ||
| /* | ||
| group changed, process the previous group's unclosed state window first | ||
| */ | ||
| doKeepCurStateWindowEndInfo(pRowSup, tsList, 0, &extendOption, false); | ||
| int32_t code = processClosedStateWindow(pInfo, pRowSup, pTaskInfo, | ||
| pExprSup, numOfOutput); | ||
| if (TSDB_CODE_SUCCESS != code) T_LONG_JMP(pTaskInfo->env, code); | ||
| *numPartialCalcRows = pRowSup->startRowIndex + pRowSup->numOfRows; | ||
|
|
||
| /* | ||
| unhandled null rows should be ignored, since they belong to previous group | ||
| */ | ||
| *numPartialCalcRows += pRowSup->numNullRows; | ||
|
|
||
| /* | ||
| reset state window info for new group | ||
| */ | ||
| pInfo->hasKey = false; | ||
| resetWindowRowsSup(pRowSup); | ||
| } | ||
|
|
||
| for (int32_t j = *startIndex; j < *endIndex; ++j) { | ||
| if (pBlock->info.scanFlag != PRE_SCAN) { | ||
| if (pInfo->winSup.lastTs == INT64_MIN || gid != pRowSup->groupId || !pInfo->hasKey) { | ||
| pInfo->winSup.lastTs = tsList[j]; | ||
| } else { | ||
| if (tsList[j] == pInfo->winSup.lastTs) { | ||
| // forbid duplicated ts rows | ||
| qError("%s:%d duplicated ts found in state window aggregation", __FILE__, __LINE__); | ||
| pTaskInfo->code = TSDB_CODE_QRY_WINDOW_DUP_TIMESTAMP; | ||
| T_LONG_JMP(pTaskInfo->env, TSDB_CODE_QRY_WINDOW_DUP_TIMESTAMP); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The removal of the duplicate timestamp check is a concern. state_window aggregation is sensitive to row order, and duplicate timestamps can lead to non-deterministic results if their processing order changes between runs. This could be a significant correctness issue. It's recommended to either restore this check or ensure that data fed into this operator has guaranteed unique timestamps.
| SML_CHECK_CODE(smlBuildTempHash(tagHashTmp, *tableMeta, (*tableMeta)->tableInfo.numOfColumns, | ||
| (*tableMeta)->tableInfo.numOfColumns + (*tableMeta)->tableInfo.numOfTags)); | ||
| SML_CHECK_CODE(smlBuildTempHash(colHashTmp, *tableMeta, 0, (*tableMeta)->tableInfo.numOfColumns)); |
There was a problem hiding this comment.
The hash tables tagHashTmp and colHashTmp are rebuilt from scratch in every iteration of this loop by calling smlBuildTempHash. This can be inefficient, especially when processing a large number of schema changes at once, as it repeatedly iterates over the entire schema. A more optimal approach would be to build the hash tables once before the loop, and then rebuild them only after a schema change has been successfully applied via changeMeta.
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.