Conversation
…queries - Fix vgArray build order in getTableMetaViaRpc: call dynMakeVgArraySortBy before checking vgArray size, since queryBuildUseDbOutput only populates vgHash and vgArray needs to be built from it on demand. - Register TDMT_VND_TABLE_META_RSP in dmTransport.c response routing table so executor's asyncSendMsgToServer callback is properly invoked. - Add comprehensive test script t.sql covering all vtable-ref-vtable scenarios including 2-layer chains, super table scans, aggregation, window queries, and data consistency checks. Made-with: Cursor
Add 30+ new test sections covering: all data types (bool/binary/nchar) through vtable chain, NULL propagation, empty source tables, mixed physical+vtable refs under same vstb, multiple vstbs sharing same source, TAG/TBNAME/PARTITION BY filters, 3-layer vtable chain, many children under one vstb, subqueries, UNION, JOIN, advanced functions (SPREAD/DIFF/TOP/BOTTOM/SAMPLE/UNIQUE), interval+FILL, state/session windows, DISTINCT, SLIMIT/SOFFSET, arithmetic expressions, CAST, string/math/time functions, DROP intermediate vtable, LAST_ROW, cross-table column refs, information_schema queries. Uses VGROUPS 4 to exercise multi-vgroup routing. Made-with: Cursor
Move TSDB_MAX_VTABLE_REF_DEPTH from local definitions in parTranslater.c and catalog.c to include/util/tdef.h as a single global definition. Replace hardcoded depth limit of 2 in dynqueryctrloperator.c and scanoperator.c with the macro. Increase the maximum virtual table reference depth from 2 to 5. Add comprehensive depth chain tests in t.sql covering layers 3/4/5, data consistency verification across all layers, vstb with children at different depths, and aggregation/window queries on the deepest chain. Made-with: Cursor
Replace the async-gated checkRefDepth with an iterative depth check using catalogGetTableMeta directly. This ensures the depth limit is enforced during CREATE VTABLE even in async parsing mode, preventing creation of vtable chains exceeding TSDB_MAX_VTABLE_REF_DEPTH. Fix colRef iteration to skip ts column (colRef[0].hasRef=0) and find the first column with an actual reference. Update t.sql: remove vstb child referencing L5 (correctly exceeds depth limit), add explicit test for depth exceeded on CREATE. Made-with: Cursor
- Add int8_t depth field to SColRef structure - Calculate and store reference depth during CREATE VTABLE - Use SColRefWrapper version (static=1) for backward compatibility with old data (version=0, depth defaults to 0) - Update all serialization paths: tmsg.h inline, tmsg.c wrapper, metaEntry.c storage, nodesCodeFuncs.c JSON - Executor optimization: skip RPC iteration in dynqueryctrloperator when depth<=1 (direct physical table reference) Made-with: Cursor
Cover vtable chain creation/query up to depth 5, vstb with vtable children, mixed physical+vtable refs, deep chain consistency, NULL propagation, empty source, window/join/union/subquery, depth limit enforcement, and DROP behavior. Made-with: Cursor
Pass the colRef depth from meta storage through the full data path (vnode -> serialization -> systable scan -> coordinator) so that resolveVtableColRefToPhysical can skip RPC resolution when depth=1 (direct physical table reference), preserving pre-feature performance. Changes across 5 layers: - SRefColInfo/SColRefInfo: add depth field - vnodeReadVSubtables: fill depth from SColRef - tmsg serialize/deserialize: encode depth with backward compat - systable schema (userVctbColsSchema): add col_ref_depth column - sysscanoperator: fill depth column in data block - planLogicCreater: extend ins_vctb_cols scan to include depth col - dynqueryctrloperator: read depth, pass to resolve, short-circuit when depth>0 && depth<=1 - parTranslater: disallow virtual super table as colRef target Made-with: Cursor
The ts column's depth is hardcoded to 0, but its reference target may be a virtual table (not physical). Using colDepth <= 1 incorrectly short- circuited for depth=0, returning the virtual table name instead of recursively resolving to the physical table. Change to colDepth == 1 so only explicitly direct-physical references skip RPC resolution. Made-with: Cursor
…ble scan The hardcoded loop bound (i=1 to 9) in addInsColumnScanCol skipped the table_name column and was fragile when new columns were added to the ins_virtual_child_columns schema. Use pMeta->numOfColumns to scan all columns dynamically, ensuring the depth column and any future additions are included in the VSTB systable scan data block. Made-with: Cursor
…engine into enh/virtualTableRef2
There was a problem hiding this comment.
Pull request overview
This PR adds end-to-end support for virtual tables referencing other virtual tables (chained vtable refs), including depth limiting/cycle detection, metadata propagation, and CI coverage to validate the behavior across query patterns.
Changes:
- Add vtable→vtable chain handling (depth tracking, (de)serialization, and runtime resolution to physical tables).
- Extend parser/planner/executor/catalog plumbing for reference depth and original-table resolution.
- Add a comprehensive pytest suite and wire it into CI.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/ci/cases.task | Adds the new virtual-table chain pytest to CI task list. |
| test/cases/05-VirtualTables/test_vtable_ref_vtable.py | New regression tests covering vtable→vtable chains (depth, windows, joins, drops, etc.). |
| t.sql | Adds a manual SQL script for exercising vtable chain behavior. |
| source/util/src/terror.c | Adds new error messages for circular refs / depth exceeded / missing origin. |
| include/util/taoserror.h | Defines new error codes for vtable chain errors. |
| include/util/tdef.h | Introduces TSDB_MAX_VTABLE_REF_DEPTH=5. |
| source/libs/qcom/src/queryUtil.c + include/libs/qcom/query.h | Extends setColRef to carry ref depth. |
| include/common/tmsg.h + source/common/src/msg/tmsg.c | Extends col-ref wire format to include depth (with compatibility guards in some paths). |
| source/dnode/vnode/src/meta/metaEntry.c | Persists colRef depth in vnode meta encoding. |
| source/dnode/vnode/src/vnd/vnodeQuery.c | Propagates depth in vsubtables refCols. |
| source/common/src/systable.c + source/libs/executor/src/sysscanoperator.c | Adds and fills a new systable column (col_ref_depth). |
| source/libs/nodes/src/nodesCodeFuncs.c | Adds JSON field for col-ref depth. |
| include/libs/nodes/querynodes.h + include/libs/nodes/cmdnodes.h | Adds refDepth fields to AST nodes/options. |
| source/libs/parser/src/parTranslater.c | Adds depth/cycle logic and resolves refs (but see comments). |
| source/libs/planner/src/planLogicCreater.c | Fixes insert-column scan loop bound to use real column count. |
| source/libs/executor/src/scanoperator.c | Adds runtime vtable-chain resolution to physical tables (but see comments). |
| source/libs/executor/src/dynqueryctrloperator.c + source/libs/executor/inc/dynqueryctrl.h | Adds RPC path to fetch table meta for resolving vtable col refs (but see comments). |
| source/libs/catalog/src/catalog.c + include/libs/catalog/catalog.h | Adds catalog helper to resolve original table vgroup across vtable chains. |
| source/dnode/mgmt/node_mgmt/src/dmTransport.c | Routes TDMT_VND_TABLE_META_RSP into qWorker response handler. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (ownedMeta) taosMemoryFree(pCurMeta); | ||
| if (code != TSDB_CODE_SUCCESS) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
In checkColRef's virtual-table depth traversal, pCurMeta may be freed before checking whether checkRefDepthGetMeta succeeded; if the call fails, the loop breaks with pCurMeta still pointing to freed memory and the final cleanup can double-free it. Reorder the free to happen only after a successful fetch, or null out pCurMeta after freeing on the error path.
| if (ownedMeta) taosMemoryFree(pCurMeta); | |
| if (code != TSDB_CODE_SUCCESS) { | |
| break; | |
| } | |
| if (code != TSDB_CODE_SUCCESS) { | |
| if (ownedMeta) { | |
| taosMemoryFree(pCurMeta); | |
| } | |
| pCurMeta = NULL; | |
| break; | |
| } | |
| if (ownedMeta) { | |
| taosMemoryFree(pCurMeta); | |
| } |
| pOrgTable->me.type = TSDB_NORMAL_TABLE; | ||
| break; |
There was a problem hiding this comment.
resolveVirtualTableToPhysical sets pOrgTable->me.type = TSDB_NORMAL_TABLE when getTableEntryByName fails (next table not on this vnode), but orgTable doesn't contain the referenced table's metadata/schema in that case. The caller immediately uses schema->nCols/schema->pSchema for column mapping, so this can produce invalid reads/crashes. Instead, return an explicit error/route indicator and let the caller handle remote routing, or fetch the referenced table meta before returning a schema pointer.
| pOrgTable->me.type = TSDB_NORMAL_TABLE; | |
| break; | |
| return code; |
| qDebug("getTableMetaViaRpc: sending to vgId:%d ep:%s:%u for table %s.%s", vgInfo->vgId, pEp->fqdn, pEp->port, dbName, tbName); | ||
|
|
||
| code = asyncSendMsgToServer(pVtbScan->pMsgCb->clientRpc, &vgInfo->epSet, NULL, pMsgSendInfo); | ||
| QUERY_CHECK_CODE(code, lino, _return); |
There was a problem hiding this comment.
getTableMetaViaRpc allocates the request buffer and SMsgSendInfo, but if asyncSendMsgToServer returns an error those allocations aren't freed (ownership is only transferred on success). Add cleanup on the asyncSend failure path to avoid leaking buf/pMsgSendInfo.
| QUERY_CHECK_CODE(code, lino, _return); | |
| if (code != TSDB_CODE_SUCCESS) { | |
| taosMemoryFree(buf); | |
| taosMemoryFree(pMsgSendInfo); | |
| QUERY_CHECK_CODE(code, lino, _return); | |
| } |
| int32_t code = checkRefDepth(pCxt, pDbName, pTableName, 1, pVisitedTables, pPrecision); | ||
|
|
||
| // Cleanup visited tables array | ||
| taosArrayDestroy(pVisitedTables); | ||
| return code; |
There was a problem hiding this comment.
getOriginalTablePrecision/checkRefDepth duplicates fullName strings into pVisitedTables, but then only calls taosArrayDestroy(pVisitedTables). taosArrayDestroy does not free pointed-to elements, so this leaks each taosStrdup() entry. Free the stored strings (e.g., iterate and taosMemoryFree) or use taosArrayDestroyP with a free callback.
Summary of ChangesHello, 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 significantly enhances the virtual table functionality by allowing virtual tables to reference other virtual tables, forming a chain of references. This feature provides greater flexibility in data modeling and abstraction. The changes include modifications to core data structures, serialization protocols, and query execution logic to correctly resolve these chained references. Robust checks for circular references and a configurable maximum reference depth have been integrated to ensure system stability and prevent infinite loops. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 introduces a significant enhancement to support virtual tables referencing other virtual tables, enabling chained or nested virtual tables. However, several memory leaks were identified in the new RPC handling and parser logic, specifically with RPC response buffers and internal tracking arrays for circular reference detection not being properly cleaned up, which could lead to memory exhaustion. Additionally, a critical bug in the parser could lead to a crash, and there's a high-severity issue regarding missing validation for circular references during virtual table creation. There are also several new but unused functions that need to be addressed.
| } | ||
|
|
||
| _return: | ||
| taosArrayDestroy(pVisitedTables); |
There was a problem hiding this comment.
The variable pVisitedTables is initialized to NULL on line 24118 and is never reassigned. Calling taosArrayDestroy(pVisitedTables) here is problematic as it operates on a NULL pointer. This appears to be leftover code from a refactoring and should be removed to prevent a potential crash and clean up the function.
| int32_t code = checkRefDepth(pCxt, pDbName, pTableName, 1, pVisitedTables, pPrecision); | ||
|
|
||
| // Cleanup visited tables array | ||
| taosArrayDestroy(pVisitedTables); |
There was a problem hiding this comment.
A memory leak was identified in getOriginalTablePrecision. The pVisitedTables array, used for circular reference detection, stores duplicated strings via taosStrdup. However, taosArrayDestroy only frees the array structure, not the string elements. You must iterate through the array and free each string before destroying the array to prevent memory exhaustion. Furthermore, the validation logic in this section has issues: the checkColRef function, while calculating reference depth, does not check for circular references, potentially allowing invalid virtual table chains. The functions detectCircularReference and checkRefDepth contain the necessary logic but are not called. Please integrate the circular reference detection logic into checkColRef.
| QUERY_CHECK_NULL(pVtbScan->pTbMetaRsp, code, lino, _return, terrno) | ||
| } | ||
|
|
||
| code = tDeserializeSTableMetaRsp(pMsg->pData, (int32_t)pMsg->len, pVtbScan->pTbMetaRsp); | ||
| QUERY_CHECK_CODE(code, lino, _return); |
There was a problem hiding this comment.
Memory leak in dynProcessTbMetaRsp. The response data buffer pMsg->pData is not freed if tDeserializeSTableMetaRsp fails or if memory allocation for pVtbScan->pTbMetaRsp fails. In TDengine's asynchronous RPC framework, the callback is responsible for freeing the message data buffer when the RPC is successful (code == TSDB_CODE_SUCCESS).
| QUERY_CHECK_NULL(pMsgSendInfo, code, lino, _return, terrno) | ||
|
|
||
| pVtbScan->pTbMetaRsp = pRsp; | ||
|
|
||
| pMsgSendInfo->param = pOperator; | ||
| pMsgSendInfo->msgInfo.pData = buf; | ||
| pMsgSendInfo->msgInfo.len = contLen; | ||
| pMsgSendInfo->msgType = TDMT_VND_TABLE_META; | ||
| pMsgSendInfo->fp = dynProcessTbMetaRsp; | ||
| pMsgSendInfo->requestId = pTaskInfo->id.queryId; | ||
|
|
||
| SEp* pEp = &vgInfo->epSet.eps[vgInfo->epSet.inUse]; | ||
| qDebug("getTableMetaViaRpc: sending to vgId:%d ep:%s:%u for table %s.%s", vgInfo->vgId, pEp->fqdn, pEp->port, dbName, tbName); | ||
|
|
||
| code = asyncSendMsgToServer(pVtbScan->pMsgCb->clientRpc, &vgInfo->epSet, NULL, pMsgSendInfo); | ||
| QUERY_CHECK_CODE(code, lino, _return); |
There was a problem hiding this comment.
Memory leak in getTableMetaViaRpc. The request buffer buf and the message send info pMsgSendInfo are leaked if asyncSendMsgToServer fails or if intermediate allocations fail. These buffers should be cleaned up in the _return block if they were not successfully handed over to the RPC layer.
| /** | ||
| * @brief Recursively get the original table's vgroup info from a virtual table chain | ||
| * @param pCxt Translate context | ||
| * @param pDbName Database name | ||
| * @param pTableName Table name (can be virtual table) | ||
| * @param pRefVgInfo Output vgroup info | ||
| * @param pVisitedTables Array of visited table names for circular reference detection | ||
| * @return TSDB_CODE_SUCCESS on success | ||
| */ | ||
| static int32_t getOriginalTableVgroupInfo(STranslateContext* pCxt, const char* pDbName, const char* pTableName, | ||
| SVgroupInfo* pRefVgInfo, SArray* pVisitedTables) { | ||
| STableMeta* pRefTableMeta = NULL; | ||
| int32_t code = TSDB_CODE_SUCCESS; | ||
| SName refName = {0}; | ||
| char fullName[TSDB_TABLE_FNAME_LEN] = {0}; | ||
|
|
||
| snprintf(fullName, sizeof(fullName), "%s.%s", pDbName, pTableName); | ||
|
|
||
| // Check circular reference | ||
| for (int32_t i = 0; i < taosArrayGetSize(pVisitedTables); i++) { | ||
| char* pVisited = *(char**)taosArrayGet(pVisitedTables, i); | ||
| if (pVisited != NULL && strcmp(pVisited, fullName) == 0) { | ||
| return generateSyntaxErrMsgExt(&pCxt->msgBuf, TSDB_CODE_VTABLE_CIRCULAR_REFERENCE, | ||
| "virtual table '%s' creates circular reference", fullName); | ||
| } | ||
| } | ||
|
|
||
| // Add current table to visited list | ||
| char* pFullNameCopy = taosStrdup(fullName); | ||
| if (pFullNameCopy == NULL || taosArrayPush(pVisitedTables, &pFullNameCopy) == NULL) { | ||
| taosMemoryFree(pFullNameCopy); | ||
| return terrno; | ||
| } | ||
|
|
||
| // Get table meta | ||
| toName(pCxt->pParseCxt->acctId, (char*)pDbName, (char*)pTableName, &refName); | ||
| code = getTableMeta(pCxt, (char*)pDbName, (char*)pTableName, &pRefTableMeta); | ||
| if (code != TSDB_CODE_SUCCESS) { | ||
| return code; | ||
| } | ||
|
|
||
| // Check if it's a virtual table | ||
| bool isVirtualTable = (pRefTableMeta->tableType == TSDB_VIRTUAL_NORMAL_TABLE || | ||
| pRefTableMeta->tableType == TSDB_VIRTUAL_CHILD_TABLE || | ||
| (pRefTableMeta->virtualStb && pRefTableMeta->tableType == TSDB_SUPER_TABLE)); | ||
|
|
||
| if (isVirtualTable && pRefTableMeta->numOfColRefs > 0 && pRefTableMeta->colRef[0].hasRef) { | ||
| // For virtual table, recursively get the original table's vgroup info | ||
| // Use the first column reference's table as the source | ||
| code = getOriginalTableVgroupInfo(pCxt, pRefTableMeta->colRef[0].refDbName, | ||
| pRefTableMeta->colRef[0].refTableName, pRefVgInfo, pVisitedTables); | ||
| taosMemoryFree(pRefTableMeta); | ||
| return code; | ||
| } else { | ||
| // For normal/child table, get vgroup info directly | ||
| code = getTableHashVgroupImpl(pCxt, &refName, pRefVgInfo); | ||
| taosMemoryFree(pRefTableMeta); | ||
| return code; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * @brief Recursively get the original table's metadata from a virtual table chain | ||
| * @param pCxt Translate context | ||
| * @param pDbName Database name | ||
| * @param pTableName Table name (can be virtual table) | ||
| * @param ppOriginalMeta Output pointer to original table meta | ||
| * @param pOriginalDbName Output original database name | ||
| * @param pOriginalTableName Output original table name | ||
| * @param pVisitedTables Array of visited table names for circular reference detection | ||
| * @return TSDB_CODE_SUCCESS on success | ||
| */ | ||
| static int32_t getOriginalTableInfo(STranslateContext* pCxt, const char* pDbName, const char* pTableName, | ||
| STableMeta** ppOriginalMeta, char* pOriginalDbName, char* pOriginalTableName, | ||
| SArray* pVisitedTables) { | ||
| STableMeta* pRefTableMeta = NULL; | ||
| int32_t code = TSDB_CODE_SUCCESS; | ||
| char fullName[TSDB_TABLE_FNAME_LEN] = {0}; | ||
|
|
||
| snprintf(fullName, sizeof(fullName), "%s.%s", pDbName, pTableName); | ||
|
|
||
| // Check circular reference | ||
| for (int32_t i = 0; i < taosArrayGetSize(pVisitedTables); i++) { | ||
| char* pVisited = *(char**)taosArrayGet(pVisitedTables, i); | ||
| if (pVisited != NULL && strcmp(pVisited, fullName) == 0) { | ||
| return generateSyntaxErrMsgExt(&pCxt->msgBuf, TSDB_CODE_VTABLE_CIRCULAR_REFERENCE, | ||
| "virtual table '%s' creates circular reference", fullName); | ||
| } | ||
| } | ||
|
|
||
| // Check depth limit | ||
| if (taosArrayGetSize(pVisitedTables) >= TSDB_MAX_VTABLE_REF_DEPTH) { | ||
| return generateSyntaxErrMsgExt(&pCxt->msgBuf, TSDB_CODE_VTABLE_REF_DEPTH_EXCEEDED, | ||
| "virtual table reference depth exceeded maximum limit %d", TSDB_MAX_VTABLE_REF_DEPTH); | ||
| } | ||
|
|
||
| // Add current table to visited list | ||
| char* pFullNameCopy = taosStrdup(fullName); | ||
| if (pFullNameCopy == NULL || taosArrayPush(pVisitedTables, &pFullNameCopy) == NULL) { | ||
| taosMemoryFree(pFullNameCopy); | ||
| return terrno; | ||
| } | ||
|
|
||
| // Get table meta | ||
| code = getTableMeta(pCxt, (char*)pDbName, (char*)pTableName, &pRefTableMeta); | ||
| if (code != TSDB_CODE_SUCCESS) { | ||
| return code; | ||
| } | ||
|
|
||
| // Check if it's a virtual table | ||
| bool isVirtualTable = (pRefTableMeta->tableType == TSDB_VIRTUAL_NORMAL_TABLE || | ||
| pRefTableMeta->tableType == TSDB_VIRTUAL_CHILD_TABLE || | ||
| (pRefTableMeta->virtualStb && pRefTableMeta->tableType == TSDB_SUPER_TABLE)); | ||
|
|
||
| if (isVirtualTable && pRefTableMeta->numOfColRefs > 0 && pRefTableMeta->colRef[0].hasRef) { | ||
| // For virtual table, recursively get the original table info | ||
| char* nextDbName = pRefTableMeta->colRef[0].refDbName; | ||
| char* nextTableName = pRefTableMeta->colRef[0].refTableName; | ||
| taosMemoryFree(pRefTableMeta); | ||
|
|
||
| return getOriginalTableInfo(pCxt, nextDbName, nextTableName, ppOriginalMeta, | ||
| pOriginalDbName, pOriginalTableName, pVisitedTables); | ||
| } else { | ||
| // For normal/child table, return the metadata | ||
| *ppOriginalMeta = pRefTableMeta; | ||
| if (pOriginalDbName != NULL) { | ||
| tstrncpy(pOriginalDbName, pDbName, TSDB_DB_FNAME_LEN); | ||
| } | ||
| if (pOriginalTableName != NULL) { | ||
| tstrncpy(pOriginalTableName, pTableName, TSDB_TABLE_FNAME_LEN); | ||
| } | ||
| return TSDB_CODE_SUCCESS; | ||
| } | ||
| } |
There was a problem hiding this comment.
These new static functions, getOriginalTableVgroupInfo and getOriginalTableInfo, appear to be unused within this file. If they are not needed for the current implementation, they should be removed to improve code maintainability and avoid confusion.
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.