yuqi1129 commented on code in PR #14057:
URL: https://github.com/apache/gravitino/pull/14057#discussion_r4345506107
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -657,17 +657,61 @@ protected JdbcTable getOrCreateTable(
return null != lazyLoadCreateTable ? lazyLoadCreateTable :
load(databaseName, tableName);
}
- protected void validateUpdateColumnNullable(
+ /**
+ * Validates a nullability change and returns the column it applies to.
+ *
+ * @param change the nullability change
+ * @param table the table that contains the column
+ * @return the current column
+ */
+ protected JdbcColumn validateUpdateColumnNullable(
TableChange.UpdateColumnNullability change, JdbcTable table) {
if (change.fieldName().length > 1) {
throw new UnsupportedOperationException("Nested column names are not
supported");
}
- String col = change.fieldName()[0];
- JdbcColumn column = getJdbcColumnFromTable(table, col);
- if (!change.nullable() && column.defaultValue().equals(Literals.NULL)) {
- throw new IllegalArgumentException(
- "column " + col + " with null default value cannot be changed to not
null");
+ return getJdbcColumnFromTable(table, change.fieldName()[0]);
+ }
+
+ /**
+ * Returns a copy of the column with the default value and nullability it
has after all default
+ * value and nullability changes to it in the same alter request are
applied. Dialects that
+ * regenerate the whole column definition for one change use it, so that the
regenerated
+ * definition does not revert another change to the same column. A NULL
default conflicts with NOT
+ * NULL, so it is dropped when the column ends up non-nullable.
+ *
+ * @param column the current column
+ * @param changes all changes in the alter request
+ * @return the column with the pending default value and nullability
+ */
+ protected static JdbcColumn withPendingDefaultValueAndNullability(
+ JdbcColumn column, TableChange... changes) {
+ Expression defaultValue = column.defaultValue();
+ boolean nullable = column.nullable();
+ for (TableChange change : changes) {
+ if (!(change instanceof TableChange.ColumnChange)) {
+ continue;
+ }
+ String[] fieldName = ((TableChange.ColumnChange) change).fieldName();
+ if (fieldName.length != 1 || !fieldName[0].equals(column.name())) {
+ continue;
+ }
+ if (change instanceof TableChange.UpdateColumnDefaultValue) {
+ defaultValue = ((TableChange.UpdateColumnDefaultValue)
change).getNewDefaultValue();
+ } else if (change instanceof TableChange.UpdateColumnNullability) {
+ nullable = ((TableChange.UpdateColumnNullability) change).nullable();
+ }
+ }
+ if (!nullable && Literals.NULL.equals(defaultValue)) {
+ defaultValue = DEFAULT_VALUE_NOT_SET;
}
+ return JdbcColumn.builder()
+ .withName(column.name())
+ .withType(column.dataType())
Review Comment:
[Important] Other full-column definitions in the same request still use the
original column state and can undo this nullability/default update.
For an original nullable INT column with `DEFAULT NULL`, the real MySQL SQL
generator produces this for `[updateColumnNullability(c, false),
updateColumnType(c, Long)]`:
```sql
ALTER TABLE `t`
MODIFY COLUMN `c` int NOT NULL COMMENT 'old',
MODIFY COLUMN `c` bigint NULL DEFAULT NULL COMMENT 'old';
```
Pairing the nullability change with a comment update similarly emits a
second definition restoring `NULL DEFAULT NULL`. Conversely, this helper copies
the original type/comment, so a later nullability definition can revert earlier
type/comment changes. These requests previously failed validation and are now
accepted.
I also executed the analogous ClickHouse sequence starting with `c
Nullable(Int32)`: `MODIFY COLUMN c Int32, MODIFY COLUMN c Nullable(Int64)
DEFAULT NULL` succeeds and leaves the column nullable.
Could we build a coherent pending column state for all same-column changes
and make every full-definition renderer use it (or emit a single final
definition per column)? Please cover combinations/orderings with type, comment,
and position changes, including the resulting column metadata. The MySQL
example above is SQL-generation evidence, not a live MySQL execution result.
##########
catalogs/catalog-jdbc-common/src/main/java/org/apache/gravitino/catalog/jdbc/operation/JdbcTableOperations.java:
##########
@@ -657,17 +657,61 @@ protected JdbcTable getOrCreateTable(
return null != lazyLoadCreateTable ? lazyLoadCreateTable :
load(databaseName, tableName);
}
- protected void validateUpdateColumnNullable(
+ /**
+ * Validates a nullability change and returns the column it applies to.
+ *
+ * @param change the nullability change
+ * @param table the table that contains the column
+ * @return the current column
+ */
+ protected JdbcColumn validateUpdateColumnNullable(
TableChange.UpdateColumnNullability change, JdbcTable table) {
if (change.fieldName().length > 1) {
throw new UnsupportedOperationException("Nested column names are not
supported");
}
- String col = change.fieldName()[0];
- JdbcColumn column = getJdbcColumnFromTable(table, col);
- if (!change.nullable() && column.defaultValue().equals(Literals.NULL)) {
- throw new IllegalArgumentException(
- "column " + col + " with null default value cannot be changed to not
null");
+ return getJdbcColumnFromTable(table, change.fieldName()[0]);
+ }
+
+ /**
+ * Returns a copy of the column with the default value and nullability it
has after all default
+ * value and nullability changes to it in the same alter request are
applied. Dialects that
+ * regenerate the whole column definition for one change use it, so that the
regenerated
+ * definition does not revert another change to the same column. A NULL
default conflicts with NOT
+ * NULL, so it is dropped when the column ends up non-nullable.
+ *
+ * @param column the current column
+ * @param changes all changes in the alter request
+ * @return the column with the pending default value and nullability
+ */
+ protected static JdbcColumn withPendingDefaultValueAndNullability(
+ JdbcColumn column, TableChange... changes) {
+ Expression defaultValue = column.defaultValue();
+ boolean nullable = column.nullable();
+ for (TableChange change : changes) {
+ if (!(change instanceof TableChange.ColumnChange)) {
+ continue;
+ }
+ String[] fieldName = ((TableChange.ColumnChange) change).fieldName();
+ if (fieldName.length != 1 || !fieldName[0].equals(column.name())) {
+ continue;
+ }
+ if (change instanceof TableChange.UpdateColumnDefaultValue) {
+ defaultValue = ((TableChange.UpdateColumnDefaultValue)
change).getNewDefaultValue();
+ } else if (change instanceof TableChange.UpdateColumnNullability) {
+ nullable = ((TableChange.UpdateColumnNullability) change).nullable();
+ }
+ }
+ if (!nullable && Literals.NULL.equals(defaultValue)) {
Review Comment:
[Important] This also silently changes an explicit request to set `DEFAULT
NULL` into a request to remove the default, even without a nullability
transition.
Starting from `c INT NOT NULL DEFAULT 5`, call only:
```java
TableChange.updateColumnDefaultValue(new String[] {"c"}, Literals.NULL)
```
The real MySQL SQL generator now emits `MODIFY COLUMN c int NOT NULL ...`
without a DEFAULT clause: this branch substitutes `DEFAULT_VALUE_NOT_SET` for
the explicitly requested `Literals.NULL`. The request therefore removes the
existing default instead of preserving the requested value or rejecting an
incompatible change.
Could we restrict cleanup to an inherited NULL default during the intended
nullable-to-NOT-NULL transition, and explicitly reject an incompatible newly
requested NULL default? Please add coverage for a standalone default update and
for an explicit NULL default combined with NOT NULL in the same batch.
`Literals.NULL` and `Column.DEFAULT_VALUE_NOT_SET` represent different user
intentions.
##########
catalogs-contrib/catalog-jdbc-clickhouse/src/main/java/org/apache/gravitino/catalog/clickhouse/operations/ClickHouseTableOperations.java:
##########
@@ -1430,20 +1431,11 @@ private String deleteIndexDefinition(
}
private String updateColumnNullabilityDefinition(
- TableChange.UpdateColumnNullability change, JdbcTable table) {
- validateUpdateColumnNullable(change, table);
+ TableChange.UpdateColumnNullability change, JdbcTable table,
TableChange... changes) {
+ JdbcColumn column = validateUpdateColumnNullable(change, table);
String col = change.fieldName()[0];
- JdbcColumn column = getJdbcColumnFromTable(table, col);
- JdbcColumn updateColumn =
- JdbcColumn.builder()
- .withName(col)
- .withDefaultValue(column.defaultValue())
- .withNullable(change.nullable())
- .withType(column.dataType())
- .withComment(column.comment())
- .withAutoIncrement(column.autoIncrement())
- .build();
+ JdbcColumn updateColumn = withPendingDefaultValueAndNullability(column,
changes);
Review Comment:
[Important] Omitting `DEFAULT` does not remove ClickHouse's existing default
expression.
For a column with an explicit `DEFAULT NULL`, this now generates the
equivalent of:
```sql
CREATE TABLE t (c Nullable(String) DEFAULT NULL) ENGINE = Memory;
ALTER TABLE t MODIFY COLUMN c String;
```
The ALTER fails locally on ClickHouse 26.1.1.712 with `Cannot convert NULL
to a non-nullable type`: ClickHouse preserves the old default when a MODIFY
supplies a type but omits the default expression. The same preservation logic
is present in the [24.8
source](https://github.com/ClickHouse/ClickHouse/blob/v24.8.14.39-lts/src/Storages/AlterCommands.cpp#L1273).
Could we implement explicit default removal using ClickHouse's semantics and
add a regression that executes the DDL and reloads the column? Checking that
the generated SQL does not contain `DEFAULT` misses this failure. If removal
requires separate statements, failure atomicity also needs consideration; on my
local version, putting `REMOVE DEFAULT` and the type change in the same ALTER
still fails.
--
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]
