SachinForNeenjas commented on issue #11805:
URL: https://github.com/apache/gravitino/issues/11805#issuecomment-5640627684
Thanks @FANNG1, this is really helpful — especially the point about
comma-separated actions. I confirmed it in the builders:
`MysqlTableOperations.generateAlterTableSql:410` and
`DorisTableOperations.generateAlterTableSql:810` both do `"ALTER TABLE \`t\`\n"
+ String.join(",\n", alterSql) + ";"`, and `PostgreSqlTableOperations:604-614`
appends `,\nALTER COLUMN ... SET NOT NULL` right after the type. So a top-level
comma does inject an extra action into the same statement, and a `;`-only
blocklist would miss it. Agreed on validating in `fromGravitino` and on not
trying to prove the type is legal.
Two cases where I think the character set needs to be slightly wider than
`[\w .,:()<>\[\]]`:
**1. `-` — the set as written would fail two existing tests.**
`TestMysqlTypeConverter.java:101` and `TestPostgreSqlTypeConverter.java:124`
both assert:
```java
checkGravitinoTypeToJdbcType(USER_DEFINED_TYPE,
Types.ExternalType.of(USER_DEFINED_TYPE));
// USER_DEFINED_TYPE = "user-defined"
```
Since `-` isn't in the set, `fromGravitino(ExternalType.of("user-defined"))`
would throw and both tests would start failing.
Suggestion: keep `-` in the set, but reject the two-character sequence `--`
separately. `--` is what actually opens a line comment; a lone hyphen can't.
`/* */` and `#` need no special case, since `/`, `*` and `#` are already
outside the set.
**2. `=` — needed for ClickHouse `Enum8`/`Enum16`.**
`Enum8('active'=1,'inactive'=2)` is stored as an `ExternalType` today
(`TestClickHouseTypeConverter.java:108-110`, `CatalogClickHouseIT.java:2934`).
ClickHouse is out of scope for this PR, but since the helper lands in the
shared `JdbcTypeConverter`, leaving `=` out now means the contrib follow-up
would have to widen it later.
My reading is that `=` is safe here: with `;` outside the set, top-level
commas rejected and brackets required balanced, a bare `=` in the column-type
position can't start a new action or terminate the statement — at worst it's a
syntax error the database rejects, which is the outcome you described for
invalid types anyway. Happy to leave it out if you'd rather keep the first PR
minimal.
So the set would become `[\w .,:()<>\[\]=-]` plus an explicit `--`
rejection, with the rest exactly as you described: strip single-quoted literals
first, `()`/`<>` balanced and never negative, commas only inside brackets.
Two smaller notes:
- `Types.ExternalType.of()` accepts `null` and `" "` today, and
`JsonUtils.readExternalType` only checks that the `catalogString` field is
present — so I'll reject null/empty/blank in the helper as well.
- For the contrib follow-up: Hologres looks more exposed than the others.
`HologresTableOperations:501` joins with `String.join("\n", alterSql)` and each
clause carries its own `;`, so a semicolon in the type string could append an
entirely new statement there rather than just an extra ALTER action.
Planning to scope this PR to MySQL, PostgreSQL and Doris plus the shared
helper, and open a follow-up for ClickHouse/OceanBase/Hologres. (FWIW StarRocks
doesn't need it — its `fromGravitino` has no `ExternalType` branch and already
throws.)
--
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]
