SEPURI-SAI-KRISHNA opened a new pull request, #13077:
URL: https://github.com/apache/seatunnel/pull/13077
Yes, glad to contribute to Apache SeaTunnel.
### Purpose of this pull request
Fixes #13075.
Two case conversions in the SQL Server JDBC code used the JVM default
locale. Under `tr-TR` and `az-AZ`, `i` upper cases to `İ`, so both broke on a
worker with such a default locale. Both now use `Locale.ROOT`.
**`SqlServerTypeConverter:114`, column types.** SQL Server type names are
lower case (`int`, `bigint`, `datetime2`), and the converter upper cased them
before its `switch`. `int` became `İNT` and failed with `COMMON-17 unsupported
convert type 'İNT'`. This one method serves the catalog (`table_path`, from
`sys.types.name`), the query path (`ResultSetMetaData.getColumnTypeName`, which
the Microsoft driver reports in lower case) and SQL Server CDC
(`SqlServerTypeUtils`). So the one change fixes all three.
**`SqlServerURLParser:53`, URL property keys.** The parser upper cases the
keys to match them case insensitively, then looks up `INSTANCENAME`. Under
`tr-TR` the key became `İNSTANCENAME`, the instance was lost, and the parser
fell back to port 1433. For
`jdbc:sqlserver://localhost;instanceName=SQLEXPRESS`, `urlWithoutDatabase` was
`jdbc:sqlserver://localhost:1433;instanceName=SQLEXPRESS;...` instead of
`jdbc:sqlserver://localhost\SQLEXPRESS;...`. The driver documentation says that
when both a port and an instance name are given, the port wins and the instance
name is ignored, so the catalog and SQL Server CDC connected to the default
instance.
`SqlServerDialect:202` (`query.toLowerCase().contains("where")`) is left as
is: `WHERE` contains no `I`, so its result is the same in every locale.
### Does this PR introduce any user facing change?
Yes, only on workers whose default locale is Turkish or Azerbaijani: SQL
Server column types convert there, and a URL with an `instanceName` property
keeps the instance. I compared `dev`'s classes with these on 105 type names
(every SQL Server type name in lower, upper and capitalised form) and 36 URLs
(each address property key in three spellings, with and without a host in the
URL):
- `tr-TR` and `az-AZ`: 43 type names that failed now convert, and the 4 URLs
whose key is spelled `instanceName` or `instancename` keep the instance.
Nothing else changes, and every result now equals the `ROOT` one.
- `ROOT`: identical results before and after.
So no working job changes, and no entry in `incompatible-changes.md` is
needed. No option, default or API changed.
### How was this patch tested?
Two tests in the existing test classes, each running under `tr-TR` and
`az-AZ` and restoring the base locale and the `FORMAT` and `DISPLAY` defaults:
- `SqlServerTypeConverterTest.testConvertDoesNotDependOnTheDefaultLocale`:
`int`, `bigint`, `datetime2`.
-
`SqlServerURLParserTest.testInstanceNamePropertyDoesNotDependOnTheDefaultLocale`:
the URL above keeps `localhost\SQLEXPRESS` and no port. The existing tests
only use the `host\instance` form, which is why they pass in a Turkish JVM.
Reverting either change, its test fails.
I also ran the whole `connector-jdbc` suite in a JVM started with
`-Duser.language=tr -Duser.country=TR`. With this change, 4 existing tests that
failed there pass (`decimal` and the three identity types), and no test starts
failing. 9 tests in `SqlServerTypeConverterTest` still fail there, but now on
the test side: they compare `column.getSourceType().toLowerCase()` with the
default locale and get, for example, `ınt`. The converter's result is correct.
The default CI locale is unaffected, so I left those assertions alone. The two
`SqlServerDialectTest` failures are the `field_ide` lookup that #13063 fixes.
There is no E2E change. The failure depends on the default locale of the
engine JVM, and the E2E suites run the engine in containers with their own
default locale.
Checks, JDK 17:
- `./mvnw clean test -pl seatunnel-connectors-v2/connector-jdbc`: 1198
tests, 0 failures, 4 errors. The 4 are the Oracle and Kingbase container tests,
which need Docker.
- `./mvnw spotless:check -pl seatunnel-connectors-v2/connector-jdbc`: clean.
- `./mvnw -B -T 1 clean test -D"license.skipAddThirdParty"=true -pl
seatunnel-ci-tools`: 34 tests, 0 failures, 2 skipped by the module.
### Check list
* [X] Code changed are covered with tests, or it does not need tests for
reason
* [X] If any new Jar binary package adding in your PR, please add License
Notice according to
[License
Notice](https://github.com/apache/seatunnel/blob/dev/docs/en/contribution/new-license.md):
none added.
* [X] If necessary, please update the documentation to describe the new
feature: not needed, no documented behaviour changes.
* [X] If you are contributing the connector code, please check that the
following files are updated:
1. Update
[plugin-mapping.properties](https://github.com/apache/seatunnel/blob/dev/plugin-mapping.properties):
not needed, no new connector.
2. Update the pom file of
[seatunnel-dist](https://github.com/apache/seatunnel/blob/dev/seatunnel-dist/pom.xml):
not needed, no new module.
3. Add ci label in
[label-scope-conf](https://github.com/apache/seatunnel/blob/dev/.github/workflows/labeler/label-scope-conf.yml):
not needed, connector-jdbc is already labelled.
4. Add e2e test for your connector: not needed, see above.
5. Update [connector-v2
doc](https://github.com/apache/seatunnel/tree/dev/docs/en/connectors): not
needed, no option or mapping changes.
* [X] Update the `release-note`: not applicable, the file was removed from
`dev` in #9304.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
