You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@shardingsphere.apache.org by "iamhucong (via GitHub)" <gi...@apache.org> on 2026/08/12 06:32:41 UTC

[PR] Parse MySQL modified column NOT NULL constraints [shardingsphere]

iamhucong opened a new pull request, #39421:
URL: https://github.com/apache/shardingsphere/pull/39421

   Fix MySQL `ALTER TABLE ... MODIFY COLUMN` nullability metadata.
   
   ### Problem
   
   The MySQL grammar accepts `NULL` and `NOT NULL` in a modified column definition, but the MODIFY visitor always constructed `ColumnDefinitionSegment` with `notNull=false`.
   
   MySQL documents `MODIFY [COLUMN] col_name column_definition` in `ALTER TABLE`: https://dev.mysql.com/doc/refman/8.4/en/alter-table.html
   
   ### Change
   
   - Detect `NOT NULL` in the MODIFY column's existing `columnAttribute` AST.
   - Assert both existing `UNSIGNED [ZEROFILL] NOT NULL` parser cases.
   - Add an explicit `NULL` counter-boundary.
   - Add the ShardingSphere 5.5.4 bug-fix release note.
   
   ### Verification
   
   - RED: the two existing MODIFY NOT NULL cases were the only failures among 1,898 MySQL parser tests before the visitor change.
   - `InternalMySQLParserIT`: 1,898 tests passed.
   - MySQL parser module: 7 tests passed.
   - Spotless passed.
   - Checkstyle reported 0 violations.
   - `git diff --check` passed.
   


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Parse MySQL modified column NOT NULL constraints [shardingsphere]

Posted by "iamhucong (via GitHub)" <gi...@apache.org>.

iamhucong commented on PR #39421:
URL: https://github.com/apache/shardingsphere/pull/39421#issuecomment-5263279780

### Result

**Review Result: Mergeable**

The change correctly fixes MySQL `ALTER TABLE ... MODIFY COLUMN` nullability metadata. No code-correctness blockers were found.

### Evidence

- Reviewed public head: `c37c35984972a14a3d2adec938d43cda78849701`.
- Before this change, the MODIFY path constructed `ColumnDefinitionSegment` through `generateColumnDefinitionSegment()` with `notNull=false` unconditionally.
- The updated visitor sets `notNull=true` only when the same `columnAttribute` contains both `NOT` and `NULL`:
- Explicit `NULL` is not misclassified as non-null.
- The separate `NOT SECONDARY` attribute is not misclassified as `NOT NULL`.
- The MySQL grammar already models the relevant column attribute as `NOT? NULL`; no grammar change is needed.
- The MySQL 8.4 reference documents both `MODIFY [COLUMN] col_name column_definition` and `ALTER TABLE t1 MODIFY b INT NOT NULL`: https://dev.mysql.com/doc/refman/8.4/en/alter-table.html
- Parser baselines cover the relevant boundaries:
- `UNSIGNED NOT NULL` → `not-null="true"`
- `UNSIGNED ZEROFILL NOT NULL` → `not-null="true"`
- Explicit `NULL` → `not-null="false"`
- `AlterTableStatementAssert` passes each modified column to `ColumnDefinitionAssert`, which directly compares `ColumnDefinitionSegment.isNotNull()`, so the test protects the production result rather than fixture shape.
- Sanitized local verification:
- RED: the two existing MODIFY NOT NULL cases were the only failures among 1,898 MySQL parser tests before the visitor change.
- GREEN: all 1,898 tests passed.
- MySQL parser module: 7 tests passed.
- Spotless passed.
- Checkstyle reported 0 violations.
- Triple-dot `git diff --check` passed.

### Coverage

- All four files in the authoritative GitHub scope were reviewed:
- `parser/sql/engine/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/engine/mysql/visitor/statement/type/MySQLDDLStatementVisitor.java`
- `test/it/parser/src/main/resources/sql/supported/ddl/alter-table.xml`
- `test/it/parser/src/main/resources/case/ddl/alter-table.xml`
- `RELEASE-NOTES.md`
- The local triple-dot file list matches GitHub at merge base `38792f442da732528d69ac050a659297068044ce`.
- Reviewed behavior cluster: MySQL MODIFY COLUMN nullability AST metadata and its ShardingSphere 5.5.4 release note.
- The review covered the ALTER visitor entry, MODIFY helper, grammar AST, adjacent CREATE/ADD/CHANGE paths, binder propagation, metadata and Encrypt consumers, assertion path, segment bounds, and release-note link.
- `MySQLDDLStatementVisitor` is an independent final dialect visitor. Doris uses a separate visitor with its own nullability extraction; no other dialect is changed.
- No public API, SPI, grammar, dependency, configuration, packaging, or generated-source contract changes are present.
- Root-cause, blast-radius, tests/runtime/operations, and final full-scope convergence passes found no unresolved evidence gaps.
- This result is code-scope only; CI was not reviewed.

--
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Parse MySQL modified column NOT NULL constraints [shardingsphere]

Posted by "FlyingZC (via GitHub)" <gi...@apache.org>.

FlyingZC merged PR #39421:
URL: https://github.com/apache/shardingsphere/pull/39421


-- 
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: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org