Conversation
Pengrongkun
requested review from
dapan1121,
guanshengliang and
hzcheng
as code owners
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
This PR enables vtable (virtual table) query support for stmt2 (prepared statement v2). Previously, vtable queries were blocked when using stmt2 with an error TSDB_CODE_VTABLE_NOT_SUPPORT_STMT. This enhancement allows stmt2 to query both virtual super tables and virtual child tables with parameterized queries.
Key changes:
- Removed the stmt version check that blocked vtable queries in stmt2
- Added graceful handling for missing VStbRefDbs metadata in stmt scenario (returns empty vgroup list for runtime resolution)
- Added comprehensive test coverage for vtable queries via stmt2 (replaces negative test with positive test)
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| source/libs/parser/src/parTranslater.c | Removed stmt version check blocking vtable queries; added error handling for missing VStbRefDbs cache data in stmt scenarios |
| source/libs/parser/src/parUtil.c | Added null checks and special handling for stmt scenario where VStbRefDbs slot is reserved but not filled |
| source/libs/planner/src/planLogicCreater.c | Set pTargets to NULL after destroy to prevent potential dangling pointer issues |
| source/libs/catalog/src/ctgCache.c | Properly set numOfColRefs and rversion fields when copying metadata for virtual child tables |
| source/libs/parser/src/parInsertSql.c | Changed log level from warning to trace for table name specified in SQL (reduces log noise) |
| source/client/test/stmt2Test.cpp | Replaced negative test case with comprehensive positive test coverage for vtable queries via stmt2; improved test cleanup |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // TODO(smj) : create a fake logic node, no need to collect column | ||
| PLAN_ERR_JRET(createScanLogicNode(pCxt, pSelect, (SRealTableNode*)nodesListGetNode(pVirtualTable->refTables, 1), &pInsColumnsScan)); | ||
| nodesDestroyList(((SScanLogicNode*)pInsColumnsScan)->node.pTargets); | ||
| ((SScanLogicNode*)pInsColumnsScan)->node.pTargets = NULL; // Set to NULL after destroy to avoid use-after-free |
There was a problem hiding this comment.
[nitpick] The comment on line 1134 mentions "avoid use-after-free", but this is not accurate. Setting a pointer to NULL after destroying it prevents double-free issues and dangling pointer access, not use-after-free specifically. Consider updating the comment to: "Set to NULL after destroy to prevent double-free or dangling pointer issues".
| ((SScanLogicNode*)pInsColumnsScan)->node.pTargets = NULL; // Set to NULL after destroy to avoid use-after-free | |
| ((SScanLogicNode*)pInsColumnsScan)->node.pTargets = NULL; // Set to NULL after destroy to prevent double-free or dangling pointer issues |
| // query virtual child table | ||
| code = taos_stmt2_prepare(stmt, "select c1 from vtb_0 where ts = ? order by c1", 0); | ||
| checkError(stmt, code, __FILE__, __LINE__); | ||
| int t2_len = 5; |
There was a problem hiding this comment.
Unused variable t2_len is declared but never used. It should be removed.
| int t2_len = 5; |
Description
Please briefly describe the code changes in this pull request.
<Close/close/Fix/fix/Resolve/resolve>:
Checklist
Please check the items in the checklist if applicable.