Skip to content

feat: stmt2 supports the decimal data type#115

Open
qevolg wants to merge 21 commits intomainfrom
feat/stmt2/decimal
Open

feat: stmt2 supports the decimal data type#115
qevolg wants to merge 21 commits intomainfrom
feat/stmt2/decimal

Conversation

@qevolg
Copy link
Copy Markdown
Contributor

@qevolg qevolg commented Apr 13, 2026

Description

feat: stmt2 supports the decimal data type.

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?

qevolg and others added 4 commits April 13, 2026 17:52
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…ror, query test

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings April 13, 2026 11:43
Copy link
Copy Markdown

@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 introduces a design document for Decimal type support in Stmt2 and adds the docs/ directory to .gitignore. The remaining changes are limited to minor code formatting and a small string case adjustment. Feedback indicates that the actual implementation described in the design document is missing from the source code changes. Additionally, adding docs/ to .gitignore is contradictory because the PR also introduces tracked files within that same directory.

Comment thread docs/specs/2026-04-13-stmt2-decimal-support-design.md Outdated
Comment thread .gitignore
Copy link
Copy Markdown

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

This pull request appears intended to add/prepare for stmt2 DECIMAL support, but the code diffs are currently limited to formatting-only changes, a minor error-message capitalization change, a new design doc, and an update to .gitignore.

Changes:

  • Reformat several TypeScript statements without changing behavior.
  • Add a design document for stmt2 DECIMAL write support.
  • Update .gitignore to ignore docs/ (which conflicts with adding docs content).

Reviewed changes

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

Show a summary per file
File Description
nodejs/src/stmt/wsStmt2.ts Adjusts error message text casing.
nodejs/src/stmt/wsProto.ts Formatting-only line wrapping changes.
nodejs/src/stmt/wsParams2.ts Formatting-only change to conditional and JSON formatting.
nodejs/src/common/utils.ts Formatting-only change in decimalToString.
nodejs/src/common/taosResult.ts Formatting-only changes in DECIMAL/DECIMAL64 decode pushes.
docs/specs/2026-04-13-stmt2-decimal-support-design.md Adds a design doc describing DECIMAL write support changes that are not present in this PR.
.gitignore Adds docs/ to ignored paths.

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

Comment thread docs/specs/2026-04-13-stmt2-decimal-support-design.md Outdated
Comment thread docs/specs/2026-04-13-stmt2-decimal-support-design.md Outdated
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Apr 13, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.48%. Comparing base (3fe094c) to head (225f35b).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #115      +/-   ##
==========================================
+ Coverage   81.33%   81.48%   +0.14%     
==========================================
  Files          33       33              
  Lines        3279     3316      +37     
  Branches      595      603       +8     
==========================================
+ Hits         2667     2702      +35     
- Misses        468      470       +2     
  Partials      144      144              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copilot AI review requested due to automatic review settings April 14, 2026 02:46
Copy link
Copy Markdown

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.


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

Comment thread nodejs/src/stmt/wsProto.ts
@qevolg qevolg force-pushed the feat/stmt2/decimal branch from 504f9fb to 740bfdd Compare April 14, 2026 06:02
Copilot AI review requested due to automatic review settings April 14, 2026 06:02
Copy link
Copy Markdown

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.


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

Comment thread nodejs/src/stmt/wsStmt2.ts
@qevolg qevolg force-pushed the feat/stmt2/decimal branch from 740bfdd to ea57bc9 Compare April 14, 2026 06:32
Copilot AI review requested due to automatic review settings April 14, 2026 06:55
@qevolg qevolg force-pushed the feat/stmt2/decimal branch from ea57bc9 to a53064a Compare April 14, 2026 06:55
Copy link
Copy Markdown

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.


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

Comment thread nodejs/src/stmt/wsStmt2.ts Outdated
Comment thread nodejs/src/stmt/wsParams2.ts
Comment thread nodejs/test/stmt/stmt2.decimal.test.ts
@qevolg qevolg changed the title Feat/stmt2/decimal feat: stmt2 supports the decimal data type. Apr 14, 2026
@qevolg qevolg changed the title feat: stmt2 supports the decimal data type. feat: stmt2 supports the decimal data type Apr 14, 2026
Copilot AI review requested due to automatic review settings April 15, 2026 01:44
Copy link
Copy Markdown

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

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

Comments suppressed due to low confidence (1)

nodejs/src/stmt/wsParams1.ts:41

  • Stmt1BindParams.addParams no longer blocks unsupported column types (e.g. BLOB/DECIMAL) now that the explicit BLOB check was removed. Because addParams is public, callers can bypass setBlob/setDecimal and silently encode unsupported types, leading to confusing runtime failures or incorrect wire encoding. Add an explicit guard in this method to throw ERR_UNSUPPORTED_TDENGINE_TYPE for unsupported stmt1 types (at least BLOB, DECIMAL, DECIMAL64), or make unsupported types impossible to pass through addParams for stmt1.
        if (
            dataType === "number" ||
            dataType === "bigint" ||
            dataType === "boolean"
        ) {

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

Comment thread nodejs/test/stmt/stmt2.decimal.test.ts
Comment thread nodejs/test/stmt/stmt2.decimal.test.ts
Copilot AI review requested due to automatic review settings April 15, 2026 05:58
Copy link
Copy Markdown

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

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

Comments suppressed due to low confidence (1)

nodejs/src/stmt/wsParams1.ts:44

  • Stmt1BindParams.addParams no longer blocks unsupported types like BLOB (and now potentially DECIMAL as well) if callers bypass the convenience methods and call addParams directly. Since stmt1 is intended to reject these types (as reflected by StmtBindParams.setBlob/setDecimal), add a guard here to throw ERR_UNSUPPORTED_TDENGINE_TYPE when columnType is BLOB/DECIMAL/DECIMAL64 (or otherwise ensure addParams cannot be used to bind unsupported stmt1 types).
    addParams(
        params: any[],
        dataType: string,
        typeLen: number,
        columnType: number
    ): void {
        if (!params || params.length == 0) {
            throw new TaosError(
                ErrorCode.ERR_INVALID_PARAMS,
                "StmtBindParams params is invalid!"
            );
        }
        if (
            dataType === "number" ||
            dataType === "bigint" ||
            dataType === "boolean"
        ) {
            this._params.push(
                this.encodeDigitColumns(params, dataType, typeLen, columnType)
            );

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

Comment thread nodejs/test/stmt/stmt2.decimal.test.ts
Comment thread nodejs/test/stmt/stmt2.decimal.test.ts
@sheyanjie-qq
Copy link
Copy Markdown
Contributor

[P2] Reconnect can make pooled connectors OPEN again without restoring the SQL session

This change introduces a new reconnect path for existing connectors, but it does not guarantee that a reconnected pooled connector is session-ready. If an idle pooled connector reconnects without replaying
conn, it can become OPEN again while still lacking a valid TDengine SQL session. That breaks the existing reuse assumption that an OPEN pooled connector is safe to hand to the next borrower. If background
reconnect for idle pooled connectors is intentional, we need an explicit session-ready check or a forced session restore before reuse.

Copilot AI review requested due to automatic review settings April 17, 2026 07:10
Copy link
Copy Markdown

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

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

Comments suppressed due to low confidence (1)

nodejs/src/stmt/wsParams1.ts:41

  • Stmt1BindParams no longer blocks BLOB at the addParams level (the previous columnType === TDengineTypeCode.BLOB guard was removed). Since setBlob() now throws an unsupported-type error for stmt1, callers can bypass that restriction by calling addParams()/addBindFieldParams() directly and end up encoding a BLOB in stmt1 anyway. Consider restoring an explicit check here (and ideally for any other stmt1-unsupported types) so stmt1 consistently rejects unsupported column types regardless of which API is used.
        if (
            dataType === "number" ||
            dataType === "bigint" ||
            dataType === "boolean"
        ) {

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

@qevolg
Copy link
Copy Markdown
Contributor Author

qevolg commented Apr 17, 2026

[P2] Reconnect can make pooled connectors OPEN again without restoring the SQL session

This change introduces a new reconnect path for existing connectors, but it does not guarantee that a reconnected pooled connector is session-ready. If an idle pooled connector reconnects without replaying conn, it can become OPEN again while still lacking a valid TDengine SQL session. That breaks the existing reuse assumption that an OPEN pooled connector is safe to hand to the next borrower. If background reconnect for idle pooled connectors is intentional, we need an explicit session-ready check or a forced session restore before reuse.

Revisions have been made based on review comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants