-
Notifications
You must be signed in to change notification settings - Fork 79
feat: round out ODBC sqlcmd compatibility with missing features and fixes #623
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
- Change from 'CPU time / elapsed time' to ODBC format: - Network packet size (bytes) - Transaction count (xacts) - Clock Time (total, avg, xacts per sec) - Move timing from runQuery() to goCommand()/exitCommand() for proper aggregate timing across 'GO n' executions - Add PrintPerformanceStatistics() method for ODBC-compatible output - Update README.md documentation - Add unit tests for statistics format
- Add pkg/sqlcmd/server_discovery.go with shared ListServers() function - Add :SERVERLIST interactive command (same functionality as -L flag) - Refactor -L flag to use shared ListServers() function - Remove duplicate listLocalServers() and parseInstances() from cmd/sqlcmd - Add :SERVERLIST to :HELP command output - Add test for :SERVERLIST command - Update README documentation
- Fix potential panic in helpCommand when args is empty non-nil slice - Fix potential panic in serverlistCommand when args is empty non-nil slice - Fix golangci-lint S1009 warning in perftraceCommand (redundant nil check)
- Add unit tests for parseInstanceData and FormatServerList in server_discovery_test.go - Add integration test for -j flag (raw errors output) in format_test.go - Add DefaultBrowserTimeout constant for server discovery - Add DefaultPacketSize constant for performance statistics - Improve PrintPerformanceStatistics documentation with output format details
- TestPrintStatisticsIntegration: executes real query and verifies stats output - TestRawErrorsIntegration: triggers real SQL error and verifies raw error format
- EXIT(query) now prompts for continuation when parentheses are unbalanced - Continuation prompt shows ' -> ' to indicate more input expected - Handles nested parentheses and quoted strings correctly - Works only in interactive mode (file-based scripts still require single line) - Adds isExitParenBalanced() helper to track paren/quote state - Adds readExitContinuation() to read additional lines until balanced - Updates README to remove the EXIT limitation from breaking changes
- Changed from UseBOM to IgnoreBOM for UTF-16LE output - ODBC sqlcmd does not write a BOM, so neither should go-sqlcmd - Updated test data files to expect no BOM - Removes this item from breaking changes documentation
There was a problem hiding this 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 go-sqlcmd to improve compatibility with the original ODBC-based sqlcmd by adding several missing features and fixing behavioral differences. The changes focus on making go-sqlcmd a more complete drop-in replacement for ODBC sqlcmd.
Changes:
- Added new interactive commands (
:HELP,:PERFTRACE,:SERVERLIST) for better usability - Implemented
-pflag for performance statistics and-jflag for raw error output - Enhanced
EXIT(query)to support multi-line input in interactive mode - Fixed Unicode output to match ODBC sqlcmd by removing BOM (breaking change)
- Refactored server discovery code into a reusable package
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/sqlcmd/commands.go | Added :HELP, :PERFTRACE, :SERVERLIST commands; enhanced EXIT command for multi-line support; integrated performance statistics |
| pkg/sqlcmd/sqlcmd.go | Added PrintStatistics flag, perftrace writer, and PrintPerformanceStatistics method |
| pkg/sqlcmd/format.go | Added rawErrors flag to support -j flag for raw error output without formatting |
| pkg/sqlcmd/server_discovery.go | New file implementing server discovery functionality (refactored from cmd/sqlcmd/sqlcmd.go) |
| pkg/sqlcmd/server_discovery_test.go | New comprehensive tests for server discovery and formatting |
| pkg/sqlcmd/commands_test.go | Added tests for new commands (help, perftrace, serverlist) and multi-line EXIT functionality |
| pkg/sqlcmd/sqlcmd_test.go | Added integration tests for -p and -j flags |
| pkg/sqlcmd/format_test.go | Added tests for raw errors formatter option |
| cmd/sqlcmd/sqlcmd.go | Integrated new flags; removed duplicate server listing code in favor of refactored package |
| cmd/sqlcmd/testdata/unicodeout*.txt | Updated test expectations for BOM removal |
| README.md | Documented new features and removed obsolete BOM documentation |
| CHANGELOG.md | New file documenting all changes including breaking changes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(instdict) == 0 { | ||
| break | ||
| } | ||
| results[strings.ToUpper(instdict["InstanceName"])] = instdict |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential panic if the instance data doesn't contain "InstanceName" key. The code accesses instdict["InstanceName"] without checking if it exists first. While this may work in practice if SQL Server always returns this field, it's safer to check for existence to prevent panics.
Consider checking: if instanceName, ok := instdict["InstanceName"]; ok { results[strings.ToUpper(instanceName)] = instdict }
| instdict := map[string]string{} | ||
| gotName := false | ||
| var name string | ||
| for _, token := range tokens { | ||
| if gotName { | ||
| instdict[name] = token | ||
| gotName = false | ||
| } else { | ||
| name = token | ||
| if len(name) == 0 { | ||
| if len(instdict) == 0 { | ||
| break | ||
| } | ||
| results[strings.ToUpper(instdict["InstanceName"])] = instdict | ||
| instdict = map[string]string{} |
Copilot
AI
Jan 25, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Variable naming inconsistency: instdict should be instDict to follow Go naming conventions (camelCase). This would be consistent with other variables in the function like gotName and outS.
| instdict := map[string]string{} | |
| gotName := false | |
| var name string | |
| for _, token := range tokens { | |
| if gotName { | |
| instdict[name] = token | |
| gotName = false | |
| } else { | |
| name = token | |
| if len(name) == 0 { | |
| if len(instdict) == 0 { | |
| break | |
| } | |
| results[strings.ToUpper(instdict["InstanceName"])] = instdict | |
| instdict = map[string]string{} | |
| instDict := map[string]string{} | |
| gotName := false | |
| var name string | |
| for _, token := range tokens { | |
| if gotName { | |
| instDict[name] = token | |
| gotName = false | |
| } else { | |
| name = token | |
| if len(name) == 0 { | |
| if len(instDict) == 0 { | |
| break | |
| } | |
| results[strings.ToUpper(instDict["InstanceName"])] = instDict | |
| instDict = map[string]string{} |
|
Closing in favor of smaller, focused PRs:
Remaining features from this PR will be split into separate PRs:
|
Summary
This PR adds several missing features and fixes to improve compatibility with the original ODBC-based sqlcmd, making go-sqlcmd a more complete drop-in replacement.
New Features
Bug Fixes
Breaking Change
Previously, go-sqlcmd wrote a UTF-16LE BOM (\FF FE) at the start of Unicode output files. This behavior differed from ODBC sqlcmd, which never wrote a BOM.
This change aligns go-sqlcmd with ODBC sqlcmd behavior. If you have scripts or processes that depend on the BOM being present in -u\ output files, you may need to adjust them.
ODBC sqlcmd Compatibility Improvements
Testing