Skip to content

Conversation

@tianzhou
Copy link
Member

  • Add rowCount to all database connectors (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server)
  • Add extractAffectedRows utility for MySQL/MariaDB result parsing
  • Update execute-sql and custom-tool-handler to use rowCount for count field
  • Frontend now displays "N rows affected" for write operations instead of "No results returned"

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings December 25, 2025 07:03
Copy link
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

This PR enhances database result reporting by adding rowCount to track affected/returned rows across all database connectors (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server). The frontend now displays "N rows affected" for successful write operations instead of "No results returned", providing better user feedback.

Key changes:

  • Added extractAffectedRows utility function for MySQL/MariaDB result parsing
  • Updated all connectors to return rowCount alongside rows
  • Modified tool handlers to use rowCount for the count field with fallback to rows.length

Reviewed changes

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

Show a summary per file
File Description
src/utils/multi-statement-result-parser.ts Added extractAffectedRows function to calculate total affected/returned rows from MySQL/MariaDB results
src/tools/execute-sql.ts Updated to use rowCount ?? rows.length for the count field
src/tools/custom-tool-handler.ts Updated to use rowCount ?? rows.length for the count field
src/connectors/sqlite/index.ts Added rowCount to return values, tracking changes for writes and rows.length for reads
src/connectors/postgres/index.ts Added rowCount to return values using PostgreSQL's native rowCount field
src/connectors/mysql/index.ts Integrated extractAffectedRows to populate rowCount
src/connectors/mariadb/index.ts Integrated extractAffectedRows to populate rowCount
frontend/src/components/tool/ResultsTable.tsx Updated to display "N rows affected" when rowCount > 0 and no rows returned
frontend/src/api/tools.ts Updated to propagate rowCount from backend count field

Comment on lines +489 to +490
// rowCount is total changes for writes, plus rows returned for reads
return { rows: allRows, rowCount: totalChanges + allRows.length };
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

The rowCount calculation for multi-statement queries is inconsistent with single-statement behavior. For single statements, rowCount represents either affected rows (for writes) OR returned rows (for reads), but not both. However, here it sums totalChanges (affected rows from writes) and allRows.length (returned rows from reads), mixing two different semantics.

This inconsistency could be confusing when displaying "N rows affected" in the frontend, as it would show a misleading count for mixed queries. Consider either:

  1. Only counting affected rows from write operations (totalChanges)
  2. Only counting returned rows from read operations (allRows.length)
  3. Documenting that multi-statement queries return a combined count
Suggested change
// rowCount is total changes for writes, plus rows returned for reads
return { rows: allRows, rowCount: totalChanges + allRows.length };
// For multi-statement queries, align rowCount semantics with single-statement behavior:
// - If the batch is read-only, rowCount is the number of rows returned.
// - If the batch includes any writes, rowCount is the total number of rows affected.
const hasReads = readStatements.length > 0;
const hasWrites = writeStatements.length > 0;
const rowCount = hasWrites ? totalChanges : allRows.length;
return { rows: allRows, rowCount };

Copilot uses AI. Check for mistakes.
Comment on lines +92 to +101
if (isMultiStatementResult(results)) {
let totalAffected = 0;
for (const result of results) {
if (isMetadataObject(result)) {
totalAffected += result.affectedRows || 0;
} else if (Array.isArray(result)) {
totalAffected += result.length;
}
}
return totalAffected;
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

The extractAffectedRows function mixes two different semantics in multi-statement queries. For metadata objects (write operations), it counts affectedRows, but for array results (SELECT queries), it counts result.length (returned rows). This means a multi-statement query with both writes and reads will return a count that combines affected rows and returned rows, which is semantically inconsistent.

For example, "INSERT INTO users VALUES (1); SELECT * FROM users;" would return rowCount = 1 (affected) + row count from SELECT. This makes it unclear what "N rows affected" means in the UI.

Consider whether rowCount should represent:

  1. Only affected rows from write operations (skip SELECT results)
  2. Only returned rows from read operations (skip write metadata)
  3. A documented combined count with clear semantics

Copilot uses AI. Check for mistakes.
Comment on lines +484 to +486
// Accumulate rowCount for INSERT/UPDATE/DELETE statements
if (result.rowCount) {
totalRowCount += result.rowCount;
Copy link

Copilot AI Dec 25, 2025

Choose a reason for hiding this comment

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

In PostgreSQL, the pg library's result.rowCount is set for both SELECT and write operations. For SELECT statements, rowCount equals the number of rows returned; for INSERT/UPDATE/DELETE, it's the number of affected rows.

This means when accumulating rowCount in multi-statement transactions that mix reads and writes, the totalRowCount will combine both returned rows and affected rows. This creates the same semantic inconsistency as in the SQLite and MySQL/MariaDB connectors, where the meaning of rowCount becomes ambiguous in mixed queries.

For consistency and clarity, consider whether to track only affected rows from write operations, or document this combined behavior explicitly.

Suggested change
// Accumulate rowCount for INSERT/UPDATE/DELETE statements
if (result.rowCount) {
totalRowCount += result.rowCount;
// Accumulate rowCount only for write operations (INSERT/UPDATE/DELETE)
if (result.command && ["INSERT", "UPDATE", "DELETE"].includes(result.command.toUpperCase())) {
totalRowCount += result.rowCount ?? 0;

Copilot uses AI. Check for mistakes.
- Add rowCount to all database connectors (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server)
- Add extractAffectedRows utility for MySQL/MariaDB result parsing
- Update execute-sql and custom-tool-handler to use rowCount for count field
- Frontend now displays "N rows affected" for write operations instead of "No results returned"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <[email protected]>
@tianzhou tianzhou merged commit 8b27f88 into main Dec 26, 2025
2 checks passed
tianzhou added a commit that referenced this pull request Dec 26, 2025
- Add rowCount to all database connectors (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server)
- Add extractAffectedRows utility for MySQL/MariaDB result parsing
- Update execute-sql and custom-tool-handler to use rowCount for count field
- Frontend now displays "N rows affected" for write operations instead of "No results returned"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.5 <[email protected]>
tianzhou added a commit that referenced this pull request Dec 26, 2025
- Add rowCount to all database connectors (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server)
- Add extractAffectedRows utility for MySQL/MariaDB result parsing
- Update execute-sql and custom-tool-handler to use rowCount for count field
- Frontend now displays "N rows affected" for write operations instead of "No results returned"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.5 <[email protected]>
tianzhou added a commit that referenced this pull request Dec 26, 2025
- Add rowCount to all database connectors (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server)
- Add extractAffectedRows utility for MySQL/MariaDB result parsing
- Update execute-sql and custom-tool-handler to use rowCount for count field
- Frontend now displays "N rows affected" for write operations instead of "No results returned"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.5 <[email protected]>
tianzhou added a commit that referenced this pull request Dec 26, 2025
- Add rowCount to all database connectors (PostgreSQL, MySQL, MariaDB, SQLite, SQL Server)
- Add extractAffectedRows utility for MySQL/MariaDB result parsing
- Update execute-sql and custom-tool-handler to use rowCount for count field
- Frontend now displays "N rows affected" for write operations instead of "No results returned"

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 4.5 <[email protected]>
@tianzhou tianzhou deleted the return_row branch December 26, 2025 15:22
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.

2 participants