Conversation
…o caller (#35104) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
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 merges changes from main into the 3.0 branch, bringing in multiple feature updates and bug fixes across the CLI, sync/raft, scalar engine, stream processing, metadata validation, tests, and documentation.
Changes:
- Add a
-H/--binary-as-hexoption totaosCLI to render non-printableBINARYvalues as0x..., plus CI coverage for it. - Improve sync assigned-leader step-down behavior by introducing
appliedIndexreporting and a configurable applied-gap threshold (syncAssignedCheckAppliedGap). - Fix a scalar-engine NULL handling crash path (
cast(null as varbinary)inBETWEENtimestamp expressions) and add regression tests; plus assorted test/doc updates.
Reviewed changes
Copilot reviewed 32 out of 32 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/shell/src/shellEngine.c | Implements non-printable BINARY hex rendering logic for CLI output/file dump. |
| tools/shell/src/shellArguments.c | Adds -H/--binary-as-hex option parsing and help text. |
| tools/shell/inc/shellInt.h | Extends SShellArgs with is_binary_as_hex. |
| test/ci/cases.task | Registers new/updated CI test cases. |
| test/cases/81-Tools/02-Taos/test_tool_taos_shell_hex.py | New test validating CLI hex rendering behavior and default-off compatibility. |
| test/cases/80-Components/01-Taosd/test_com_taosd_self_audit_switch_off.py | New/added component coverage for audit self-save switching. |
| test/cases/80-Components/01-Taosd/test_com_taosd_audit_no_token.py | New telemetry audit test variant with auditUseToken=0. |
| test/cases/80-Components/01-Taosd/test_com_taosd_audit.py | Adjusts audit user/token creation to use cus_audit. |
| test/cases/70-Cluster/test_cluster_arbitrator_applied_index.py | New cluster test targeting applied-index gap behavior during assigned leader stepdown. |
| test/cases/70-Cluster/test_cluster_arbitrator.py | Docstring step order update. |
| test/cases/09-DataQuerying/02-Filter/test_scalar_null_varbinary_between.py | New regression test for NULL-varbinary BETWEEN crash. |
| source/libs/sync/src/syncAppendEntriesReply.c | Adds applied-gap threshold check before stepping down from assigned leader. |
| source/libs/sync/src/syncAppendEntries.c | Populates appliedIndex in append-entries replies. |
| source/libs/sync/inc/syncMessage.h | Extends SyncAppendEntriesReply with appliedIndex. |
| source/common/src/tglobal.c | Introduces tsSyncAssignedCheckAppliedGap config with dynamic support. |
| include/common/tglobal.h | Exposes tsSyncAssignedCheckAppliedGap. |
| docs/en/14-reference/01-components/01-taosd.md | Documents syncAssignedCheckAppliedGap. |
| docs/zh/14-reference/01-components/01-taosd.md | Documents syncAssignedCheckAppliedGap (ZH). |
| source/libs/scalar/src/scalar.c | Prevents NULL dereference by handling NULL value nodes in timestamp conversion. |
| source/libs/new-stream/src/streamTriggerTask.c | Adjusts history logic for some trigger types (notably COUNT path). |
| source/dnode/vnode/src/meta/metaTable2.c | Adds validation for virtual child table column reference counts. |
| source/dnode/mnode/impl/src/mndMain.c | Offsets SS auto-migrate scheduling to reduce conflicts with trim task timing. |
| source/dnode/mnode/impl/src/mndDnode.c | Uses configured audit user when fetching active token instead of hardcoding "audit". |
| source/dnode/mnode/impl/src/mndDb.c | Always creates audit STB when creating an audit DB. |
| source/client/src/clientImpl.c | Clears request error/message when treating “table not exist” as empty result. |
| docs/node.md(x) | Node connector docs updated for BLOB mapping and stmt bind setBlob. |
| docs/taos-cli.md | Documents new -H/--binary-as-hex CLI option. |
| docs/tdgpt | Updates anomaly-detection algo input docs for multi-column support (self.input_data_lists). |
Comments suppressed due to low confidence (1)
source/libs/new-stream/src/streamTriggerTask.c:11753
- readAllData is set to true unconditionally, so the
if (!readAllData)metadata-acceleration path is now dead code and any potential optimization is disabled. If correctness requires always reading full data, consider removing the dead branch to avoid confusion; otherwise restore the previous conditional so COUNT checks can still use metadata fast-path when safe.
bool readAllData = true;
bool allTableProcessed = false;
bool needFetchData = false;
#define ALIGN_UP(x, b) (((x) + (b) - 1) / (b) * (b))
while (!allTableProcessed && !needFetchData) {
if (!readAllData) {
// use table metadatas to accelerate the count window check
if (IS_TRIGGER_TIMESTAMP_SORTER_EMPTY(pContext->pSorter)) {
stTimestampSorterReset(pContext->pSorter);
pContext->pCurTableMeta = tSimpleHashIterate(pGroup->pTableMetas, pContext->pCurTableMeta, &pContext->tbIter);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (platform.system().lower() == 'windows' and not tdDnodes.dnodes[0].remoteIP == ""): | ||
| RequestHandlerImplStr = base64.b64encode(pickle.dumps(RequestHandlerImpl)).decode() | ||
| cmdStr = "import pickle\nimport http\nRequestHandlerImpl=pickle.loads(base64.b64decode(\"%s\".encode()))\nclass NewRequestHandlerImpl(RequestHandlerImpl):\n hostPort = \'%s\'\nhttp.server.HTTPServer((\"\", %s), NewRequestHandlerImpl).serve_forever()"%(RequestHandlerImplStr,hostname+":"+serverPort,telemetryPort) | ||
| tdDnodes.dnodes[0].remoteExec({}, cmdStr) |
There was a problem hiding this comment.
base64 is used to serialize RequestHandlerImpl on Windows, but base64 is not imported. This will raise NameError on the Windows branch; add the missing import or avoid base64 usage here.
| while True: | ||
| tdLog.info("createThreadExited = %s"%createThreadExited) | ||
| if createThreadExited == True: | ||
|
|
||
| break | ||
|
|
||
| time.sleep(5) |
There was a problem hiding this comment.
createTbThread waits in an unbounded while True loop for createThreadExited. If telemetry callbacks never arrive (network issue / audit config issue), this test will hang CI indefinitely. Add a timeout/max retry and fail the test with tdLog.exit when exceeded.
| while True: | |
| tdLog.info("createThreadExited = %s"%createThreadExited) | |
| if createThreadExited == True: | |
| break | |
| time.sleep(5) | |
| wait_timeout = 60 | |
| wait_interval = 5 | |
| deadline = time.monotonic() + wait_timeout | |
| while time.monotonic() < deadline: | |
| tdLog.info("createThreadExited = %s"%createThreadExited) | |
| if createThreadExited == True: | |
| break | |
| time.sleep(wait_interval) | |
| else: | |
| tdLog.exit("Timed out waiting for createThreadExited after %s seconds" % wait_timeout) |
| if (shell.args.is_binary_as_hex && shellHasBinaryNonPrintable(val, length)) { | ||
| char *tmp = shellAllocHexString(val, length); | ||
| if (tmp == NULL) break; | ||
| taosFprintfFile(pFile, "%s%s%s", quotationStr, tmp, quotationStr); | ||
| taosMemoryFree(tmp); | ||
| break; | ||
| } |
There was a problem hiding this comment.
If shellAllocHexString() fails, the code breaks out of the switch without printing any field value. That will silently produce malformed CSV output (missing column content). Consider falling back to the existing printable-path serialization (NCHAR/JSON style) or printing a clear placeholder/error so each column always emits some output.
| static void shellPrintHex(const char *val, int32_t length, int32_t width) { | ||
| char *hexBuf = shellAllocHexString(val, length); | ||
| if (hexBuf == NULL) return; | ||
|
|
||
| shellPrintString(hexBuf, width > 0 ? width : 0); | ||
|
|
There was a problem hiding this comment.
shellPrintHex() returns early on allocation failure, so the caller prints nothing for that column and the table output becomes misaligned. It would be more robust to fall back to shellPrintNChar() (or at least print an explicit placeholder) when hex buffer allocation fails.
| int32_t pos = 0; | ||
| while (pos < length) { | ||
| TdWchar wc; | ||
| int32_t remain = length - pos; | ||
| int32_t bytes = taosMbToWchar(&wc, val + pos, TMIN(MB_CUR_MAX, remain)); | ||
| if (bytes <= 0) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
shellHasBinaryNonPrintable() calls taosMbToWchar(), which uses mbtowc() (global static conversion state on glibc). When bytes<=0, returning immediately can leave mbtowc's internal state corrupted and break later multibyte conversions in the same thread. Consider using mbrtowc() with a local mbstate_t for validation, or explicitly resetting the mbtowc state (e.g., mbtowc(NULL, NULL, 0)) before returning on error.
| static char *shellAllocHexString(const char *val, int32_t length) { | ||
| int32_t hexLen = 2 + length * 2; | ||
| char *hexBuf = (char *)taosMemoryCalloc(1, hexLen + 1); | ||
| if (hexBuf == NULL) { | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
shellAllocHexString() computes hexLen = 2 + length * 2 in int32_t and allocates based on that. For large length, this can overflow and lead to undersized allocation / buffer overwrite in shellHexEncode. Consider using size_t for the calculation and add an overflow check before allocating.
| createTableReceived = False | ||
| insertReceived = False | ||
| selectReceived = False | ||
| deleteReceived = False | ||
| class RequestHandlerImpl(http.server.BaseHTTPRequestHandler): | ||
| hostPort = hostname + ":" + serverPort | ||
|
|
||
| def telemetryInfoCheck(self, infoDict=''): | ||
| global createThreadExited | ||
| global createTableReceived | ||
| global insertReceived | ||
| global selectReceived | ||
| global deleteReceived |
There was a problem hiding this comment.
createThreadExited is referenced as a global (e.g., in telemetryInfoCheck/createTbThread) but is never defined/initialized in this file. This will raise NameError at runtime; initialize it (e.g., createThreadExited = False) at module scope alongside the other *_Received flags.
Description
Issue(s)
Checklist
Please check the items in the checklist if applicable.