Skip to content

fix(planner): verify %%tbname to use single-table external window#35054

Merged
guanshengliang merged 1 commit intomainfrom
fix/external-window-placeholder-single-table
Apr 6, 2026
Merged

fix(planner): verify %%tbname to use single-table external window#35054
guanshengliang merged 1 commit intomainfrom
fix/external-window-placeholder-single-table

Conversation

@JinqingKuang
Copy link
Copy Markdown
Contributor

Description

fix(planner): verify %%tbname to use single-table external window

Issue(s)

Checklist

Please check the items in the checklist if applicable.

  • Is the user manual updated?
  • Are the test cases passed and automated?
  • Is there no significant decrease in test coverage?

Copilot AI review requested due to automatic review settings April 3, 2026 07:07
@taosdata-bot taosdata-bot Bot added the internal label Apr 3, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 enhances stream query processing by correctly identifying single-table windows when the asSingleTable flag is set or when partitioning by table name. Key changes include the addition of helper functions and a new test case in parStreamTest.cpp, as well as logic updates in planLogicCreater.c. A review comment points out an inconsistency in createWindowLogicNodeByStreamExternal, suggesting the addition of a partition-by-tbname check to match the logic in createWindowLogicNodeByExternal.

Comment on lines +2484 to +2485
if (pTable->pMeta->tableType == TSDB_NORMAL_TABLE || pTable->pMeta->tableType == TSDB_CHILD_TABLE ||
pTable->asSingleTable) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic for determining isSingleTable in createWindowLogicNodeByStreamExternal is inconsistent with createWindowLogicNodeByExternal. While createWindowLogicNodeByExternal (line 2434) checks for both isPartTb (partition by tbname) and asSingleTable, this function only checks for asSingleTable. For consistency and to ensure that stream windows partitioned by table name are correctly identified as single-table windows even when the %%tbname placeholder is not explicitly used in the FROM clause, the partition check should be added here as well.

    if (pTable->pMeta->tableType == TSDB_NORMAL_TABLE || pTable->pMeta->tableType == TSDB_CHILD_TABLE ||
        pTable->asSingleTable || (pSelect->pPartitionByList && keysHasTbname(pSelect->pPartitionByList))) {

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes planner single-table detection for external windows when the query uses the %%tbname placeholder (single-table external window should be selected in this case), addressing the linked defect.

Changes:

  • Treat SRealTableNode::asSingleTable as a sufficient condition to mark external window logic nodes as isSingleTable (both regular and stream external-window paths).
  • Add a parser test that builds a stream query using %%tbname, deserializes the generated calc plan, and asserts the external window physical node is marked isSingleTable.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
source/libs/planner/src/planLogicCreater.c Extends external-window single-table detection to honor asSingleTable for real tables.
source/libs/parser/test/parStreamTest.cpp Adds regression test validating %%tbname produces a single-table external window in the calc plan.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@JinqingKuang
Copy link
Copy Markdown
Contributor Author

代码审查报告

审查状态: completed
已验证问题: 1 个(低危)


整体评估

修复逻辑正确。在 createWindowLogicNodeByExternalcreateWindowLogicNodeByStreamExternal 两处均补充了 pTable->asSingleTable 条件,覆盖了 %%tbname placeholder 触发单表外部窗口的场景。新增测试覆盖完整的 parser → planner → 物理计划反序列化验证链路,回归保护有效。


已验证问题

[LOW] 测试回调中 ASSERT_* 在分配资源后提前返回会跳过清理代码

GTest 的 ASSERT_* 宏在 lambda 内触发失败时执行 return 退出 lambda,导致后续的 nodesDestroyNode(pPlanNode)tFreeSCMCreateStreamReq(&req) 不会执行。

req 在第 1706 行分配后,第 1707、1708、1711、1712、1715 行的 ASSERT_* 调用均有此问题。详见 inline comment。

修复方向:req 分配之后、清理代码之前的 ASSERT_NE / ASSERT_EQ 改为 EXPECT_NE / EXPECT_EQ(测试失败后继续执行到清理代码),或使用 RAII 保证清理。


观察(历史遗留,非本 PR 引入,已过滤)

createWindowLogicNodeByStreamExternal 缺少 isPartTb 检查,与 createWindowLogicNodeByExternal 不一致。此差异在本 PR 之前已存在,Gemini 审查已有说明,供后续跟进参考。


SNode* pPlanNode = nullptr;
ASSERT_EQ(TSDB_CODE_SUCCESS, nodesStringToNode((char*)req.calcPlan, &pPlanNode));
ASSERT_NE(pPlanNode, nullptr);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

req(第 1706 行)分配后,ASSERT_* 在 GTest lambda 中失败时执行 return 直接退出 lambda,导致下方的 nodesDestroyNode(pPlanNode)tFreeSCMCreateStreamReq(&req) 不会被执行。

同样的问题也存在于第 1707、1708、1711、1712 行的 ASSERT_* 调用。

建议: 将这几处 ASSERT_NE / ASSERT_EQ 改为 EXPECT_NE / EXPECT_EQ,使测试在断言失败后仍能执行到清理代码:

// 改为 EXPECT 系列,确保 cleanup 一定被执行
EXPECT_EQ(TSDB_CODE_SUCCESS, tDeserializeSCMCreateStreamReq(..., &req));
EXPECT_NE(req.calcPlan, nullptr);
if (!req.calcPlan) { tFreeSCMCreateStreamReq(&req); return; }
// ...

或者把清理逻辑提前到一个 RAII guard 中(如 std::unique_ptr + 自定义 deleter)。

@guanshengliang guanshengliang merged commit 395d03f into main Apr 6, 2026
16 of 17 checks passed
@JinqingKuang JinqingKuang deleted the fix/external-window-placeholder-single-table branch April 7, 2026 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants