SEPURI-SAI-KRISHNA opened a new pull request, #13023:
URL: https://github.com/apache/seatunnel/pull/13023
Yes, glad to contribute to Apache SeaTunnel.
### Purpose of this pull request
Closes #13022.
`DatabendCatalog.convertDatabendType` resolved two Databend column types to
the wrong SeaTunnel
type. The Databend source has no way around this method:
`DatabendSourceFactory.createSource` calls
`catalog.getTable(tablePath)` on every job and `getTable` calls
`convertDatabendType` once per
column.
**DECIMAL resolved to STRING.** Databend's `information_schema.columns` view
defines its column as
`LOWER(data_type) AS data_type`, and `data_type` comes from
`TableDataType::sql_name()`, which
formats a decimal as `Decimal(p, s)`. The driver passes that straight
through as `TYPE_NAME`, so a
`DECIMAL(10, 2)` column reached the switch as `decimal(10, 2)`. Compared
against the bare literal
`"DECIMAL"` it never matched, so `new DecimalType(columnSize,
decimalDigits)` was unreachable and
the column fell through to the STRING fallback. The precision and scale were
available all along:
the driver parses them out of the name into `COLUMN_SIZE` and
`DECIMAL_DIGITS`, and this class
already reads both.
**BINARY resolved to TINYINT.** `BasicType.BYTE_TYPE` is `new
BasicType<>(Byte.class,
SqlType.TINYINT)`, a single byte rather than a byte string. Three places
already disagreed with it:
`docs/en/connectors/source/Databend.md` and the zh copy both document
`BINARY | BYTES`;
`toDatabendTypeString` in this same class writes SeaTunnel `BYTES` back out
as `VARBINARY`; and
`DatabendSourceReader.convertDatabendTypeToSeaTunnelType` returns
`PrimitiveByteArrayType.INSTANCE`
for the same column. That divergence is what reaches the user, because
`DatabendSource.getProducedCatalogTables()` falls back to the catalog table
while `reader` is still
null, which is the case at plan time
(`MultipleTableJobConfigParser.getProducedType` calls it on the
plan-time source). The plan was therefore built on the catalog's types while
the reader emitted
values typed by its own: `BigDecimal` for a column advertised as STRING,
`byte[]` for a column
advertised as a single `Byte`.
**The switch was also locale dependent.** Since the names arrive lower
cased, `typeName
.toUpperCase()` with the default locale maps every `i` to a dotted capital I
under `tr-TR` and
`az-AZ`, and the name stops matching its own branch. Measured against the
names Databend reports,
7 of the 12 base types degraded to STRING on such a worker: `tinyint`,
`smallint`, `int`, `bigint`,
`timestamp`, `binary` and `decimal`.
### Does this PR introduce any user facing change?
Yes, and it is the point of the change: a Databend `DECIMAL` column is now
read as `DecimalType`
with the column's own precision and scale instead of as STRING, and a
`BINARY` or `VARBINARY`
column is read as BYTES instead of TINYINT. Both now agree with the
documented mapping, so no
documentation change was needed. No option, default or SPI signature changed.
### How was this patch tested?
`DatabendCatalogTest` is new. There was no existing catalog test to extend
in this module, and the
mapping is a pure function, so `convertDatabendType` was made package
private to let the test assert
it directly rather than through a live server.
Five tests, and four of them fail against the code as it stood before this
change, which is the
check that they are pinning the fix and not the bug:
```
DatabendCatalogTest.decimalKeepsItsPrecisionAndScale
expected: <Decimal(10, 2)> but was: <STRING>
DatabendCatalogTest.binaryMapsToBytesRatherThanASingleByte
expected: <BYTES> but was: <TINYINT>
DatabendCatalogTest.theDocumentedMappingHolds
unexpected SeaTunnel type for Databend type binary ==> expected: <BYTES>
but was: <TINYINT>
DatabendCatalogTest.theMappingIsIndependentOfTheDefaultLocale
unexpected SeaTunnel type for Databend type tinyint ==> expected:
<TINYINT> but was: <STRING>
```
The fifth, `anUnknownTypeStillFallsBackToString`, passes both before and
after, since that fallback
is deliberately unchanged.
Run locally on JDK 17:
- `./mvnw test -pl seatunnel-connectors-v2/connector-databend` : 20 tests, 0
failures.
- `./mvnw -B -T 1 test -D"license.skipAddThirdParty"=true -pl
seatunnel-ci-tools -am` : 0 failures.
- `./mvnw spotless:apply` : clean.
`DatabendIT` is extended as well, since this connector has an E2E.
`databend_init.conf` gains a
`type_mapping_table` holding a `DECIMAL(10, 2)` and a `BINARY` column, and
`testDatabendTypeMapping`
runs a job over it with no schema declared, so the run depends on the types
the catalog resolves.
The E2E additions are purely additive and leave the existing tables and
tests untouched. I could not
run them here because Docker is unavailable in my environment, so CI is
their first execution.
### 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)
* [X] If necessary, please update the documentation to describe the new
feature.
The documented mapping already matched the corrected behaviour, so no
change was required.
* [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)
and add new connector information in it: 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-databend is already labelled.
4. Add e2e test for your connector: done,
`DatabendIT.testDatabendTypeMapping`.
5. Update [connector-v2
doc](https://github.com/apache/seatunnel/tree/dev/docs/en/connectors):
not needed, the existing doc already states the corrected mapping.
* [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]
