-
Notifications
You must be signed in to change notification settings - Fork 5k
feat(parser): [6659773700] relax window query SELECT list restrictions #35226
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
base: 3.0
Are you sure you want to change the base?
Changes from all commits
34651f2
2d8fb51
4f4723d
35d7bbd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1900,9 +1900,106 @@ static int32_t createWindowLogicNodeFinalize(SLogicPlanContext* pCxt, SSelectStm | |
| pSelect->hasIndefiniteRowsFunc ? fmIsWindowIndefRowsFunc : fmIsWindowClauseFunc, | ||
| &pWindow->pFuncs)); | ||
|
|
||
| PLAN_ERR_JRET(rewriteExprsForSelect(pWindow->pFuncs, pSelect, SQL_CLAUSE_WINDOW, NULL)); | ||
| // Detect projection-only mode: pFuncs is NULL or contains only pseudo-column and _group_key functions | ||
| // (_group_key comes from HAVING tag columns that need slots for condition evaluation) | ||
| bool projectionMode = true; | ||
| if (pWindow->pFuncs != NULL) { | ||
| SNode* pNode = NULL; | ||
| FOREACH(pNode, pWindow->pFuncs) { | ||
| if (nodeType(pNode) == QUERY_NODE_FUNCTION) { | ||
| SFunctionNode* pFunc = (SFunctionNode*)pNode; | ||
| if (!fmIsPseudoColumnFunc(pFunc->funcId) && !fmIsGroupKeyFunc(pFunc->funcId)) { | ||
| projectionMode = false; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (projectionMode && pSelect->hasIndefiniteRowsFunc) { | ||
| // When used as subquery, the outer query may prune pProjectionList to contain only | ||
| // unnamed VALUE placeholders (e.g., SELECT count(*) FROM (subquery)). In that case, | ||
| // use the child node's first target (primary key) as a minimal projection to ensure | ||
| // the window operator still produces output rows. | ||
| bool hasNamedExpr = false; | ||
| SNode* pTmp = NULL; | ||
| FOREACH(pTmp, pSelect->pProjectionList) { | ||
| if (nodeType(pTmp) == QUERY_NODE_COLUMN || ((SExprNode*)pTmp)->aliasName[0] != '\0') { | ||
| hasNamedExpr = true; | ||
| break; | ||
| } | ||
| } | ||
|
|
||
| if (hasNamedExpr) { | ||
| PLAN_ERR_JRET(nodesCloneList(pSelect->pProjectionList, &pWindow->pProjs)); | ||
| } else { | ||
| // pProjectionList is degenerate (pruned by outer query) — use child's first target | ||
| // as minimal projection to ensure window produces output rows | ||
| SNode* pFirstTarget = nodesListGetNode(pCxt->pCurrRoot->pTargets, 0); | ||
| if (pFirstTarget != NULL) { | ||
| SNode* pClone = NULL; | ||
| PLAN_ERR_JRET(nodesCloneNode(pFirstTarget, &pClone)); | ||
| PLAN_ERR_JRET(nodesListMakeStrictAppend(&pWindow->pProjs, pClone)); | ||
|
Comment on lines
+1941
to
+1942
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential memory leak: if PLAN_ERR_JRET(nodesCloneNode(pFirstTarget, &pClone));
int32_t code = nodesListMakeStrictAppend(&pWindow->pProjs, pClone);
if (code != TSDB_CODE_SUCCESS) {
nodesDestroyNode(pClone);
return code;
} |
||
| } | ||
| } | ||
|
|
||
| // Merge remaining pFuncs entries (pseudo columns + _group_key) into pProjs. | ||
| // These come from HAVING/ORDER BY expressions that reference tags or pseudo columns | ||
| // not in SELECT — the physi creator needs output slots for them. | ||
| if (pWindow->pFuncs != NULL) { | ||
| SNode* pFuncNode = NULL; | ||
| FOREACH(pFuncNode, pWindow->pFuncs) { | ||
| bool alreadyInProjs = false; | ||
| SNode* pProjNode = NULL; | ||
| FOREACH(pProjNode, pWindow->pProjs) { | ||
| if (nodesEqualNode(pProjNode, pFuncNode)) { | ||
| alreadyInProjs = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!alreadyInProjs) { | ||
| SNode* pClone = NULL; | ||
| PLAN_ERR_JRET(nodesCloneNode(pFuncNode, &pClone)); | ||
| PLAN_ERR_JRET(nodesListMakeStrictAppend(&pWindow->pProjs, pClone)); | ||
|
Comment on lines
+1962
to
+1963
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential memory leak: if PLAN_ERR_JRET(nodesCloneNode(pFuncNode, &pClone));
int32_t code = nodesListMakeStrictAppend(&pWindow->pProjs, pClone);
if (code != TSDB_CODE_SUCCESS) {
nodesDestroyNode(pClone);
return code;
} |
||
| } | ||
| } | ||
| } | ||
|
|
||
| PLAN_ERR_JRET(createColumnByRewriteExprs(pWindow->pFuncs, &pWindow->node.pTargets)); | ||
| // Collect column references from HAVING that may not be in pProjs | ||
| // (e.g., HAVING col NOT in SELECT, or SELECT pruned by outer subquery). | ||
| if (pSelect->pHaving != NULL) { | ||
| SNodeList* pHavingCols = NULL; | ||
| PLAN_ERR_JRET(nodesCollectColumnsFromNode(pSelect->pHaving, NULL, COLLECT_COL_TYPE_ALL, &pHavingCols)); | ||
| SNode* pColNode = NULL; | ||
| FOREACH(pColNode, pHavingCols) { | ||
| bool found = false; | ||
| SNode* pProjNode = NULL; | ||
| FOREACH(pProjNode, pWindow->pProjs) { | ||
| if (nodesEqualNode(pProjNode, pColNode)) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| if (!found) { | ||
| SNode* pClone = NULL; | ||
| PLAN_ERR_JRET(nodesCloneNode(pColNode, &pClone)); | ||
| PLAN_ERR_JRET(nodesListMakeStrictAppend(&pWindow->pProjs, pClone)); | ||
|
Comment on lines
+1985
to
+1986
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential memory leak: if PLAN_ERR_JRET(nodesCloneNode(pColNode, &pClone));
int32_t code = nodesListMakeStrictAppend(&pWindow->pProjs, pClone);
if (code != TSDB_CODE_SUCCESS) {
nodesDestroyNode(pClone);
nodesDestroyList(pHavingCols);
return code;
} |
||
| } | ||
| } | ||
| nodesDestroyList(pHavingCols); | ||
|
Comment on lines
+1972
to
+1989
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| } | ||
|
Comment on lines
+1970
to
+1990
|
||
|
|
||
| // Discard pFuncs (pseudo-cols and _group_key are now in pProjs) | ||
| nodesDestroyList(pWindow->pFuncs); | ||
| pWindow->pFuncs = NULL; | ||
|
|
||
| PLAN_ERR_JRET(rewriteExprsForSelect(pWindow->pProjs, pSelect, SQL_CLAUSE_WINDOW, NULL)); | ||
| PLAN_ERR_JRET(createColumnByRewriteExprs(pWindow->pProjs, &pWindow->node.pTargets)); | ||
| } else { | ||
| // Existing function-collection path | ||
| PLAN_ERR_JRET(rewriteExprsForSelect(pWindow->pFuncs, pSelect, SQL_CLAUSE_WINDOW, NULL)); | ||
| PLAN_ERR_JRET(createColumnByRewriteExprs(pWindow->pFuncs, &pWindow->node.pTargets)); | ||
| } | ||
|
|
||
| if (NULL != pSelect->pHaving && !havingHandledByFill) { | ||
| PLAN_ERR_JRET(nodesCloneNode(pSelect->pHaving, &pWindow->node.pConditions)); | ||
|
|
||
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.
The check for
hasNamedExpris too restrictive. It currently only considers raw columns or aliased expressions as "named". This will cause unaliased scalar functions (e.g.,SELECT sin(current) FROM ...) or scalar expressions (e.g.,SELECT current + 1 FROM ...) to be incorrectly treated as degenerate placeholders, leading to them being replaced by a minimal projection (primary key) and causing subsequent evaluation failures in parent nodes.