FlyingZC commented on PR #40205:
URL: https://github.com/apache/shardingsphere/pull/40205#issuecomment-6284568575
• ### Result
Review Result: Mergeable
No blocking implementation regressions were identified within the review
scope. The two fixture limitations described below are treated as non-blocking
in this
assessment.
### Evidence
- CREATE TABLE and ALTER TABLE constraint alternatives take precedence
over generic column definitions, preventing named constraints from being
classified as
columns.
- Constraint visitors preserve constraint names and source ranges, extract
primary-key columns only for primary-key constraints, and retain referenced
tables for
foreign keys.
- CREATE TRIGGER now records both the ON table and optional FROM table
through the existing table collection and statement attributes.
- Optional JAXB cardinality attributes strengthen zero/nonzero assertions
while preserving the existing behavior of fixtures that omit those attributes.
- The three invalid constraint-trigger SQL cases and their corresponding
expectations have been removed without changing the remaining cases.
- The focused PostgreSQL ParserIT regression completed with **1,863 tests,
0 failures, 0 errors, and 0 skipped tests**. Its source and resource inputs
match the
reviewed candidate.
Two positive fixtures remain parser-only inputs and must not be
interpreted as native PostgreSQL compatibility evidence:
- `alter_table_modify_constraint_validation_pg`: PostgreSQL rejects `ALTER
CONSTRAINT ... NOT VALID`.
- `create_table_repeated_enforced_pg`: PostgreSQL rejects consecutive `NOT
ENFORCED ENFORCED` clauses.
These limitations concern fixture validity. They do not establish a newly
introduced production-parser regression or require general semantic validation
in the
parser.
### Coverage
- **Candidate:** Public PR #40205, head
`fe606e225d670aeca821a7a959511c3dc1c0f6c7`, base/merge-base
`8a10a2f4822841b45561796ad02c4709ae69e2e2`. The local checkout
matches the public head and has no additional changes.
- **Scope:** All 15 changed files, 30 changed blocks, and 63 added SQL
cases were reviewed. Coverage includes grammar precedence, constraint and
trigger AST
extraction, downstream table consumers, shared cardinality assertions, and
SQL/expected-result pairing.
- **Behavior boundary:** The changes remain within the existing PostgreSQL
parser and AST responsibilities. No new database, SPI, or runtime
semantic-validation
contract is introduced. PostgreSQL has no maintained branch dialect in the
repository’s dialect map; openGauss uses independent grammar and visitors.
- **References:** Repository coding standards and ParserIT contracts,
together with PostgreSQL 18 [CREATE
TABLE](https://www.postgresql.org/docs/18/sql-
createtable.html), [ALTER
TABLE](https://www.postgresql.org/docs/18/sql-altertable.html), [CREATE
INDEX](https://www.postgresql.org/docs/18/sql-createindex.html),
and [CREATE
TRIGGER](https://www.postgresql.org/docs/18/sql-createtrigger.html)
documentation and corresponding official source.
- **Review completion:** Root-cause and behavior, contract impact, and
test-validity analysis were completed across the full scope. Independent
assessment and
final convergence identified no additional blocking implementation
findings. No unresolved evidence gaps remain.
- **Style verification:** Existing Spotless and Checkstyle results remain
applicable: all 26 recorded source, configuration, and tool inputs are
unchanged. Both
checks exited with code 0, and their output confirms selection of all five
applicable Java files:
-
`parser/sql/engine/dialect/postgresql/src/main/java/org/apache/shardingsphere/sql/parser/engine/postgresql/visitor/statement/type/
PostgreSQLDDLStatementVisitor.java`
-
`test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/asserts/segment/definition/ConstraintDefinitionAssert.java`
-
`test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/asserts/statement/ddl/standard/type/AlterTableStatementAssert.java`
-
`test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/cases/parser/jaxb/segment/impl/definition/
ExpectedConstraintDefinition.java`
-
`test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/cases/parser/jaxb/statement/ddl/standard/table/
AlterTableStatementTestCase.java`
- **Style command, with exact path filters abbreviated:** `mvnw -B -ntp
-pl parser/sql/engine/dialect/postgresql,test/it/parser -DspotlessFiles=<five
exact file
expressions> -Dcheckstyle.includes=<matching source-root-relative paths>
-Dcheckstyle.includeResources=false -Dcheckstyle.includeTestResources=false
spotless:check checkstyle:check -Pcheck -T1`.
- **Not applicable:** The remaining G4/XML files are outside the
configured Spotless and Checkstyle input scope.
- **CI scope:** This is a code-correctness assessment. GitHub Actions and
required-check status were 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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
