yuqi1129 commented on code in PR #14665:
URL: https://github.com/apache/gravitino/pull/14665#discussion_r4479766860
##########
catalogs/catalog-jdbc-postgresql/src/main/java/org/apache/gravitino/catalog/postgresql/operation/PostgreSqlTableOperations.java:
##########
@@ -618,13 +618,24 @@ private String updateColumnTypeFieldDefinition(
if (null == column) {
throw new NoSuchColumnException("Column %s does not exist.", col);
}
+ Type newType = updateColumnType.getNewDataType();
StringBuilder sqlBuilder = new StringBuilder(ALTER_TABLE +
quotedIdentifier(jdbcTable.name()));
sqlBuilder
.append("\n")
.append(ALTER_COLUMN)
.append(quotedIdentifier(col))
.append(" SET DATA TYPE ")
-
.append(typeConverter.fromGravitino(updateColumnType.getNewDataType()));
+ .append(typeConverter.fromGravitino(newType));
+ // SET DATA TYPE resets the column to the new type's default collation.
When the column has an
+ // explicit non-default collation and the new type is still collatable
text, re-apply it so a
+ // type change does not silently drop the collation. Non-text targets must
not carry COLLATE;
+ // PostgreSQL rejects it.
+ if (isCollatableTextType(newType)) {
+ String collation = explicitColumnCollation(schemaName, jdbcTable.name(),
col);
+ if (collation != null) {
+ sqlBuilder.append(" COLLATE ").append(quotedIdentifier(collation));
Review Comment:
[Important] Could we preserve the collation's schema as well as its name?
`explicitColumnCollation()` currently selects only `collation_name`, so this
emits an unqualified `COLLATE "name"`. Collations are schema objects, and the
execution connection sets its search path to the table's schema.
I reproduced two outcomes with PostgreSQL 13 through
`TABLE_OPERATIONS.alterTable()`:
1. A varchar column uses `source_schema.custom_collation`, with that schema
outside the execution search path: widening varchar(10) to varchar(50) fails
with `collation "custom_collation" for encoding "UTF8" does not exist`.
2. The table's schema also contains a collation with the same name: the
ALTER succeeds, but `information_schema.columns.collation_schema` changes from
the original schema to the table's schema. Comparing only `collation_name`
would miss this identity change.
Please select both `collation_schema` and `collation_name`, and emit
separately quoted identifiers: `COLLATE "schema"."collation"` (rather than
quoting the combined string as one identifier). Regression tests should cover
both cases and compare the schema or collation OID. This stays within the
existing varchar/type-change scope.
Reference: PostgreSQL exposes the two identity components separately in
[information_schema.columns](https://www.postgresql.org/docs/13/infoschema-columns.html).
The two additional temporary regression assertions both failed as described;
no REST/CLI/Spark end-to-end test was run.
--
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]
