Skip to content

Conversation

@dlevy-msft-sql
Copy link
Contributor

Summary

Implements the -f\ flag from ODBC sqlcmd for specifying input and output file code pages.

Changes

  • cmd/sqlcmd/sqlcmd.go: Added \CodePage\ and \ListCodePages\ fields, -f/--code-page\ flag, validation, and runtime application
  • pkg/sqlcmd/codepage.go: Comprehensive codepage support with parsing and encoding conversion
  • pkg/sqlcmd/codepage_test.go: Unit tests for codepage parsing and supported encodings
  • pkg/sqlcmd/commands.go: Updated :R\ command to respect codepage settings when reading files
  • pkg/sqlcmd/sqlcmd.go: Added \CodePage\ field to Sqlcmd struct
  • cmd/sqlcmd/sqlcmd_test.go: Command line argument tests for valid and invalid codepage values
  • README.md: Documentation with format examples

Usage

\\�ash

Single codepage for both input and output

sqlcmd -f 65001 -i script.sql -o results.txt

Different codepages for input and output

sqlcmd -f i:1252,o:65001 -i windows_script.sql -o utf8_results.txt

List all supported codepages

sqlcmd --list-codepages
\\

Supported Codepages

  • Unicode: 65001 (UTF-8), 1200 (UTF-16LE), 1201 (UTF-16BE)
  • Windows: 874, 1250-1258
  • OEM/DOS: 437, 850, etc.
  • ISO-8859: 28591-28606
  • CJK: 932 (Shift-JIS), 936 (GB2312), 949 (Korean), 950 (Big5)
  • EBCDIC: 37, 1047, 1140

Testing

  • All existing tests pass
  • Added command line argument parsing tests
  • Added unit tests for codepage parsing and validation

Improves ODBC sqlcmd compatibility.

- Add -f/--code-page flag with ODBC-compatible format parsing

- Support 50+ codepages: Unicode, Windows, OEM/DOS, ISO-8859, CJK, EBCDIC, Macintosh

- Apply input codepage in IncludeFile() for :r command

- Apply output codepage in outCommand() for :OUT file writes

- Add --list-codepages flag to display all supported codepages

- Add comprehensive unit tests for parsing and encoding lookup
Copy link

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

Adds ODBC sqlcmd-compatible -f/--code-page support to control input/output file encodings (plus --list-codepages) for improved interoperability.

Changes:

  • Introduces codepage parsing/validation and a mapping from Windows codepages to Go encodings.
  • Applies configured codepages when reading -i / :R files and when writing -o / :OUT / :ERROR outputs.
  • Adds CLI argument tests, unit tests for codepage parsing/lookup, and README documentation.

Reviewed changes

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

Show a summary per file
File Description
cmd/sqlcmd/sqlcmd.go Adds -f/--code-page, --list-codepages, validation, and wiring into runtime Sqlcmd configuration.
cmd/sqlcmd/sqlcmd_test.go Adds CLI parsing/validation test cases for the new flags.
pkg/sqlcmd/codepage.go Implements ParseCodePage, GetEncoding, and SupportedCodePages.
pkg/sqlcmd/codepage_test.go Adds unit tests for parsing and encoding lookup.
pkg/sqlcmd/commands.go Applies output codepage transforms in :OUT and :ERROR.
pkg/sqlcmd/sqlcmd.go Applies input codepage transforms (or BOM-based auto-detect fallback) when including files.
README.md Documents -f usage and --list-codepages.

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

Comment on lines +376 to +377
encoder := transform.NewWriter(o, enc.NewEncoder())
s.SetError(encoder)
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

In errorCommand, GetEncoding(65001) returns a nil encoding (used to mean UTF-8). Calling enc.NewEncoder() will panic when OutputCodePage is 65001. Handle the nil-encoding case the same way outCommand does (set the writer directly with no transform) before calling NewEncoder().

Suggested change
encoder := transform.NewWriter(o, enc.NewEncoder())
s.SetError(encoder)
if enc == nil {
// UTF-8 (or default) encoding: write directly without transform
s.SetError(o)
} else {
encoder := transform.NewWriter(o, enc.NewEncoder())
s.SetError(encoder)
}

Copilot uses AI. Check for mistakes.
} else if s.CodePage != nil && s.CodePage.OutputCodePage != 0 {
// Use specified output codepage
enc, err := GetEncoding(s.CodePage.OutputCodePage)
if err != nil {
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

outCommand opens the output file before calling GetEncoding, but if GetEncoding returns an error the file handle is returned without being closed. Close the file (or call GetEncoding before opening the file) on that error path to avoid leaking descriptors.

Suggested change
if err != nil {
if err != nil {
_ = o.Close()

Copilot uses AI. Check for mistakes.
}

for _, tt := range tests {
t.Run(string(rune(tt.codepage)), func(t *testing.T) {
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

TestGetEncoding uses string(rune(tt.codepage)) as the subtest name, which produces non-human-readable (and potentially confusing) test names. Use a decimal string representation (e.g., fmt.Sprintf("%d", tt.codepage) or strconv.Itoa) instead.

Copilot uses AI. Check for mistakes.
if s.CodePage != nil && s.CodePage.OutputCodePage != 0 {
enc, err := GetEncoding(s.CodePage.OutputCodePage)
if err != nil {
o.Close()
Copy link

Copilot AI Jan 25, 2026

Choose a reason for hiding this comment

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

File handle may be writable as a result of data flow from a call to OpenFile and closing it may result in data loss upon failure, which is not handled explicitly.

Suggested change
o.Close()
if cerr := o.Close(); cerr != nil {
return fmt.Errorf("%v; additionally, closing error file %q failed: %w", err, args[0], cerr)
}

Copilot uses AI. Check for mistakes.
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