LeonYoah commented on PR #10427:
URL: https://github.com/apache/seatunnel/pull/10427#issuecomment-3828600383
> 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` 和 `KingbaseTypeMapper`。 However, the
new classes introduced in this PR use `KingBase` (CamelCase 'K' and 'B'), such
as `KingBaseCatalog` 和 `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.
Thank you for your review. I will make the changes.
--
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]
