fix(collation): support legacy CP437 and CP850 code pages - #452
Conversation
|
Thanks, this looks really solid. I ran it against real SQL Server 2022/2019/2017: all 24 CP437/CP850 collations in
Please let me know if I'm wrong on any assumptions. I look forward to working with you on this! |
|
Thanks for checking the collations and pointing out the gaps in the assertions. Updated in d68ce87. The all-byte regression now compares against fixed expected Unicode strings for CP437 and CP850, rather than relying only on a round trip. Swapping two table entries now fails that test. The bulk-input regression also requires the exact I also added query regression cases for both code pages across CHAR, VARCHAR, VARCHAR(MAX), TEXT, and SQL_VARIANT, including parameter round trips and checks of the stored bytes. The library suite passes with 869 tests under |
Client::bulk_insert* declared each column of the INSERT BULK column list as `[name] type` without a collation. SQL Server reads the bulk data of a char, varchar or text column declared that way in the database default collation and converts it to the column's collation, so text bulk-loaded into a column whose collation differs from the database default was stored corrupted. In a CP1252 database, "Привет" bulk-loaded into a Cyrillic_General_CI_AS column was stored as "I?eaao", and every byte 0x80-0xFF of text bulk-loaded into a SQL_Latin1_General_CP437_BIN or SQL_1xCompat_CP850_CI_AS column was stored as a different byte: CP437 "é" (0x82) became "," (0x2C). The INSERT BULK column list now declares char, varchar, text, nchar, nvarchar and ntext columns with ` COLLATE <name>`, as SqlClient's SqlBulkCopy does. The collation names come from `EXEC <catalog>..sp_tablecollations_100 N'<schema>.<table>'`, run in the batch of the column metadata query, so bulk_insert takes no extra round trip. It runs in tempdb for a # temp table and in the catalog the table name gives otherwise; before SQL Server 2008 the procedure is sp_tablecollations_90. Columns are matched to their collation by name. A collation name that is not ASCII letters, digits and `_` fails the bulk insert with Error::Protocol instead of reaching the statement.
MattJackson
left a comment
There was a problem hiding this comment.
Thanks a lot for d68ce87. Both fixes do exactly what I hoped. I swapped the first two CP437 table entries and legacy_codepages_match_unicode_mappings_for_every_byte failed right away, and with the new message check the bulk-reject test now fails on main as it should. fmt, clippy, MSRV 1.88, all 10 integration-matrix builds with -Dwarnings and the lib suites are all green, and your new query test passes on SQL Server 2022, 2019 and 2017.
I also owe you a correction. In my first comment I said bulk insert came back byte-exact, and that was wrong. The cause isn't your code, though. Main's INSERT BULK column list never declares a COLLATE, so SQL Server reads char/varchar/text bulk data in the database's default code page and converts it to the column's. That already corrupts bulk loads into any non-default-collation column on main (a Cyrillic column turns Привет into I?eaao), and CP437/CP850 columns hit the same bug: every byte 0x80–0xFF came back different (CP437 é was stored as ,).
Rather than have you chase that, I pushed a commit on top of your branch (dbb20e3). It declares each character column's collation in INSERT BULK, the way SqlClient's SqlBulkCopy does, and adds a live test that bulk-loads every high byte into CP437 and CP850 columns and checks the stored bytes. That test fails without the fix and passes with it on 2022, 2019 and 2017. With it, the full bulk (132) and query (200) suites pass on all three.
Approving. Deferring the fast path and the code_page() accessor is completely fine. Feel free to look over my commit, and tell me if you'd rather handle it differently.
Summary
CHAR,VARCHAR,TEXT, and intrinsicCHAR/VARCHARvalues insideSQL_VARIANT; use the same codecs forCHAR/VARCHAR/TEXTbulk encoding.Collation::encoding()API and built-inencoding_rsbehavior. The public accessor still returns an error for DOS code pages becauseencoding_rs::Encodingcannot represent them; internal row/bulk paths now support them.Why
For example,
SQL_1xCompat_CP850_CI_ASreports LCID0x409and sort ID49. These SQL sort IDs already appear in the collation mapping, but their arms are commented out becauseencoding_rsdoes not implement CP437/CP850. Consequently, even valid row values fail withunsupported encodingbefore the caller can read them.The sort-ID groups follow Microsoft's JDBC collation mappings. The byte tables follow the Unicode Consortium's CP437 and CP850 mappings. All 256 mappings per code page were cross-checked against those tables and Python's corresponding codec, including
0xFF→U+00A0.Regression coverage
CHAR,VARCHAR, andTEXT.SQL_VARIANTcharacter values, empty/NULL values, unrepresentable bulk values, encoded-byte length limits, built-in encodings, unknown sort IDs, and collation display names.Validation
cargo fmt --all -- --check— passed.cargo nextest run --lib --no-default-features— 869 passed.cargo nextest run --lib --no-default-features --features tds73— 913 passed.cargo nextest run --lib --no-default-features --features rustls,chrono,time,tds73— 998 passed.cargo clippy --lib --no-default-features --features rustls,chrono,time,tds73— completed with the same two existing warnings as the unmodified base (needless_borrowinclient/connection.rsandlarge_enum_variantinclient/tls.rs).-D warningsalso fails on the base for those warnings.--no-default-features --features rustls,chrono,time,tds73: 31 passed, 1 ignored, 7 failed because their examples require a live SQL Server (ConnectionRefused). An isolated run against unmodified base7fd75bbreproduced exactly the same seven failures.Unit/raw-TDS-frame and doctest validation only; no live SQL Server run.