Skip to content

Conversation

cad2040
Copy link

@cad2040 cad2040 commented Sep 2, 2025

Summary of Changes

  1. Added two new enum values to MssqlType:
    • NVARCHAR(Types.NVARCHAR, sqlStringOverride = "NVARCHAR(MAX)")
    • NVARCHAR_INDEX(Types.NVARCHAR, sqlStringOverride = "NVARCHAR(200)")
  2. Updated the conversion logic to use NVARCHAR instead of VARCHAR for string types
  3. Maintained backward compatibility by keeping the existing VARCHAR types

What

Allowing the connector to correctly handle Unicode characters, address issue 64496

How

By updating conversion logic of strings to use nvarchar instead of varchar

Review guide

  1. airbyte-integrations/connectors/destination-mssql/src/main/kotlin/io/airbyte/integrations/destination/mssql/v2/convert/AirbyteTypeToMssqlType.kt

User Impact

Positive Impact:

  • Fixes character encoding corruption - Special characters like parentheses (, ), em dashes —, and smart quotes " will no longer be corrupted during MSSQL bulk insert operations
  • Improves data integrity - Unicode characters will be properly preserved and displayed correctly in destination MSSQL tables
    Eliminates data quality issues - Text fields will maintain their original meaning and readability
  • Better internationalization support - Non-English characters and special symbols will work correctly
    No Negative Side Effects:
  • Backward compatible - Existing VARCHAR columns remain unchanged
  • Performance neutral - NVARCHAR has minimal performance impact compared to VARCHAR
  • No breaking changes - Existing syncs will continue to work
  • Standards compliant - Uses proper Unicode data types as recommended by Microsoft

Can this PR be safely reverted and rolled back?

  • YES 💚

Summary of Changes
1. Added two new enum values to MssqlType:
   - NVARCHAR(Types.NVARCHAR, sqlStringOverride = "NVARCHAR(MAX)")
  - NVARCHAR_INDEX(Types.NVARCHAR, sqlStringOverride = "NVARCHAR(200)")
2. Updated the conversion logic to use NVARCHAR instead of VARCHAR for string types
3. Maintained backward compatibility by keeping the existing VARCHAR types
@cad2040 cad2040 requested a review from a team as a code owner September 2, 2025 07:53
@CLAassistant
Copy link

CLAassistant commented Sep 2, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Contributor

github-actions bot commented Sep 2, 2025

👋 Welcome to Airbyte!

Thank you for your contribution from cad2040/airbyte! We're excited to have you in the Airbyte community.

Helpful Resources

PR Slash Commands

As needed or by request, Airbyte Maintainers can execute the following slash commands on your PR:

  • /format-fix - Fixes most formatting issues.
  • /bump-version - Bumps connector versions.
  • /run-connector-tests - Runs connector tests.
  • /run-cat-tests - Runs CAT tests.
  • /build-connector-images - Builds and publishes a pre-release docker image for the modified connector(s).

If you have any questions, feel free to ask in the PR comments or join our Slack community.

Tips for Working with CI

  1. Pre-Release Checks. Please pay attention to these, as they contain standard checks on the metadata.yaml file, docs requirements, etc. If you need help resolving a pre-release check, please ask a maintainer.
    • Note: If you are creating a new connector, please be sure to replace the default logo.svg file with a suitable icon.
  2. Connector CI Tests. Some failures here may be expected if your tests require credentials. Please review these results to ensure (1) unit tests are passing, if applicable, and (2) integration tests pass to the degree possible and expected.
  3. (Optional.) BYO Connector Credentials for tests in your fork. You can optionally set up your fork with BYO credentials for your connector. This can significantly speed up your review, ensuring your changes are fully tested before the maintainers begin their review.

📝 Edit this welcome message.

@cad2040
Copy link
Author

cad2040 commented Sep 3, 2025

The "Format Check" failure is due to a broken GitHub Action dependency (abinoda/slack-action repository not found), not because of my code changes. The actual formatting check never ran due to this setup failure.

The Airbyte team needs to update their workflow to use a working Slack action or remove the Slack notification step.

My code changes are ready for review once the workflow issue is resolved.

This fix addresses issue 64496

@davinchia
Copy link
Contributor

@cad2040 it looks like the build has proper failures. Please take a look!

Updated testConvertStringType() to expect nvarchar and testConvertIndexedStringType() to expect nvarchar
@cad2040
Copy link
Author

cad2040 commented Sep 4, 2025

@cad2040 it looks like the build has proper failures. Please take a look!

Thanks I have updated the failing unit tests to now expect NVARCHAR and not VARCHAR.

@cad2040
Copy link
Author

cad2040 commented Sep 4, 2025

Okay, it is now failing due to a missing credential in a secret file, which I think is related to the build pipelines

@cgardens
Copy link
Contributor

cgardens commented Sep 7, 2025

/run-connector-tests

Connector CI Tests Started

These tests will leverage Airbyte's integration test credentials.

Check job output.
✅ Connector CI Tests job completed successfully. See logs for details.

@DanyloGL DanyloGL moved this from Waiting Eng Team to Planned in 🧑‍🏭 Community Pull Requests Oct 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Shield Team: In Review

Development

Successfully merging this pull request may close these issues.

6 participants