diqiu50 commented on PR #14632:
URL: https://github.com/apache/gravitino/pull/14632#issuecomment-6659967830
Thanks for the fix. A few issues before merging:
1. **Function defaults read back as wrong literals**
(`PostgreSqlColumnDefaultValueConverter.java:98`): `VALUE_WITH_TYPE` uses an
unanchored `find()`, so `upper('x'::text)` on a text column loads as the string
literal `x`. Please anchor the match, fall back to `UnparsedExpression`, and
add a create-then-load test.
2. **`E'...'` decoding looks unreachable**
(`PostgreSqlColumnDefaultValueConverter.java:102-108`): on PostgreSQL 13,
catalog reads never returned an `E` prefix, so the comment is inaccurate. With
`standard_conforming_strings=off`, `a\b` reads back as `a\\b`. Please remove
the branch or decode based on the setting.
3. **`ExternalType` validation is too strict**
(`PostgreSqlTypeConverter.java:56-66`): valid declarations such as
`geometry(Point,4326)`, `national character(3)`, and `int ARRAY` are now
rejected. Can we support these forms, or at least identifier modifiers for
PostGIS?
4. **Scope of the shared converter change**: it also affects MySQL, Doris,
StarRocks, OceanBase, and Hologres, for example `abs(a)` is now rejected, but
only the PostgreSQL docs are updated. ClickHouse still bypasses this
validation, and `ExternalType` remains unvalidated in the other JDBC catalogs.
Please document the scope or open a follow-up issue.
--
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]
