davidzollo commented on PR #10427:
URL: https://github.com/apache/seatunnel/pull/10427#issuecomment-3828547271
Hi @LeonYoah, thanks for this contribution! Adding Catalog support for
Kingbase is a great enhancement.
After reviewing the code, I have a few suggestions to improve code
consistency and compatibility:
## 1. Naming Inconsistency
The existing classes in the project typically use `Kingbase` (only 'K'
capitalized), such as `KingbaseDialect` and `KingbaseTypeMapper`.
However, the new classes introduced in this PR use `KingBase` (CamelCase 'K'
and 'B'), such as `KingBaseCatalog` and `KingBaseCatalogFactory`.
**Suggestion:** Please rename the new classes to match the existing
convention:
- `KingBaseCatalog` -> `KingbaseCatalog`
- `KingBaseCatalogFactory` -> `KingbaseCatalogFactory`
- `KingBaseCreateTableSqlBuilder` -> `KingbaseCreateTableSqlBuilder`
## 2. Compatibility Risk with `rownum`
In `KingBaseCatalog.java`:
```java
@Override
protected String getExistDataSql(TablePath tablePath) {
return String.format(
"select * from \"%s\".\"%s\" WHERE rownum = 1",
tablePath.getSchemaName(), tablePath.getTableName());
}
```
**Risk:** `rownum` is an Oracle-specific syntax. While Kingbase supports an
Oracle compatibility mode, it usually runs in Postgres compatibility mode by
default, where `rownum` might not be supported or behaves differently. This
could cause the catalog to fail in environments running in standard PG mode.
**Suggestion:** Unless `rownum` is guaranteed to work in all Kingbase modes,
it is safer to use the standard SQL standard or Postgres syntax: `LIMIT 1`.
```java
"select * from \"%s\".\"%s\" LIMIT 1"
```
## 3. Documentation
It would be helpful to update the `Features` section in
`docs/en/connectors/source/Kingbase.md` to indicate that Kingbase now supports
the Catalog feature.
--
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]
