You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by zh...@apache.org on 2026/04/02 06:39:13 UTC
(shardingsphere) branch master updated: Refactor MySQLStatementVisitor createRowConstructorList logic (#38586)
This is an automated email from the ASF dual-hosted git repository.
zhaojinchao pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shardingsphere.git
The following commit(s) were added to refs/heads/master by this push:
new 39d885bd646 Refactor MySQLStatementVisitor createRowConstructorList logic (#38586)
39d885bd646 is described below
commit 39d885bd646d695ccc9f9726666b30770e494539
Author: Zhengqiang Duan <du...@apache.org>
AuthorDate: Thu Apr 2 14:39:07 2026 +0800
Refactor MySQLStatementVisitor createRowConstructorList logic (#38586)
* Refactor DorisStatementVisitor and MySQLStatementVisitor createRowConstructorList logic
* Modify SKILL.md to check db doc for sql parser
* Remove doris from replace_with_batch_and_row_constructor and insert_with_batch_and_row_constructor
* Revert DorisStatementVisitor
* Add startIndex and stopIndex in values.xml
---
.codex/skills/review-pr/SKILL.md | 36 ++++++++++++++++++++++
.../visitor/statement/MySQLStatementVisitor.java | 6 ++--
.../segment/insert/InsertValuesClauseAssert.java | 10 +++++-
.../parser/src/main/resources/case/dml/insert.xml | 4 +--
.../parser/src/main/resources/case/dml/replace.xml | 4 +--
.../parser/src/main/resources/case/dml/values.xml | 6 ++--
.../main/resources/sql/supported/dml/insert.xml | 2 +-
.../main/resources/sql/supported/dml/replace.xml | 2 +-
.../main/resources/sql/supported/dml/values.xml | 4 +--
9 files changed, 60 insertions(+), 14 deletions(-)
diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md
index f32d5f6fd1a..c4863ea4be9 100644
--- a/.codex/skills/review-pr/SKILL.md
+++ b/.codex/skills/review-pr/SKILL.md
@@ -56,6 +56,17 @@ description: >-
- one config-disabled or feature-flag-off path,
- one original symptom path that is only partially covered by tests.
If any of these remain unresolved, set `Merge Verdict: Not Mergeable`.
+15. If a PR changes SQL parser behavior, grammar, visitor logic, supported SQL cases, or parser tests for a dialect,
+ you must complete both syntax-validity review and dialect-family impact review before concluding:
+ - Every SQL syntax added, changed, accepted, rejected, or suggested in the review must be backed by the target database's official documentation for the exact database family and relevant version.
+ - This applies both to the PR's changed SQL syntax and to any SQL syntax examples or recommendations you propose in review comments.
+ - Follow the SQL parsing maintenance relationships defined by the repo conventions, for example `MySQL -> MariaDB, Doris`.
+ - If the PR changes a trunk dialect parser, inspect whether each branch dialect that reuses or copies that parser logic has the same root cause, regression risk, or missing validation.
+ - If the PR changes a branch dialect parser, inspect whether the same root cause also exists in the corresponding trunk dialect or sibling branch dialects because of shared or copied grammar / visitor logic.
+ - For each related dialect, classify it as: `same issue confirmed`, `not applicable with evidence`, or `unresolved`.
+ - If official documentation support is missing, ambiguous, or contradicts the PR behavior, bias to `Merge Verdict: Not Mergeable`.
+ - If any related dialect remains unresolved, or the review skips the family scan, bias to `Merge Verdict: Not Mergeable`.
+ - Do not recommend unsupported or undocumented SQL syntax in review feedback.
## Execution Boundary
@@ -70,6 +81,17 @@ Priority from high to low:
3. ShardingSphere official docs and official repo conventions.
4. External authoritative specs only when necessary (official standards/docs only).
+For SQL parser reviews:
+
+- Prefer the target database's official SQL reference/manual as first-class evidence.
+- When the PR or your review comment mentions a concrete SQL syntax form, cite the exact official doc page that supports that form.
+- If the official docs do not clearly support the syntax, do not infer support from another dialect, secondary article, or parser implementation alone.
+- Build a dialect-family evidence set when parser logic changes:
+ - Check repo conventions for trunk/branch relationships first.
+ - If the touched dialect is a trunk parser, inspect affected branch dialect parser files, tests, and doc paths.
+ - If the touched dialect is a branch parser, inspect the trunk parser and any sibling branch dialects that may share or copy the same logic.
+- Check ShardingSphere docs/examples/release notes when parser behavior changes, especially if the PR alters supported syntax, unsupported syntax, or dialect-specific examples.
+
Forbidden sources:
- Unverifiable blogs, forum posts, or AI-reposted content.
@@ -90,6 +112,8 @@ Before deep review, answer:
4. Are there file changes unrelated to the stated goal?
5. Does the patch change documented semantics such as name resolution, default schema, fallback precedence, or routing order?
6. Is there at least one counterexample or negative scenario validated, not only the reported happy path?
+7. If SQL parser is changed, has the review checked related trunk / branch dialects and confirmed whether the same issue also exists there?
+8. If SQL parser is changed, do official docs and ShardingSphere docs both support the new behavior, or is a doc update required?
Triage policy:
@@ -108,6 +132,7 @@ When information gaps block mergeability, request at least:
- Minimal reproducible input (SQL/request/config snippet) with expected vs actual behavior.
- Key logs or stack traces.
- Test evidence mapped one-to-one with fix points (new or adjusted tests).
+- For SQL parser reviews: official documentation links/pages for the exact syntax and version, plus any affected ShardingSphere doc paths or examples.
## Review Workflow
@@ -120,12 +145,18 @@ When information gaps block mergeability, request at least:
- Compatibility: behavior/config/API-SPI/SQL dialect versions
- Regression: similar statements, adjacent features, exception branches
- For parser, binder, routing, and default-schema changes, explicitly compare the new behavior against official dialect semantics and check whether precedence or shadowing rules changed
+ - For SQL parser changes, build the dialect-family map first using repo conventions, then expand the review to related trunk / branch dialects that reuse, share, or copy the touched parser logic
+ - For SQL parser changes, verify every changed syntax form in the PR against the target database's official documentation, and reject syntax support that cannot be proven from official docs
+ - For SQL parser review comments, ensure every suggested SQL syntax example or recommended acceptance/rejection rule is also supported by the target database's official documentation; do not suggest parser behavior that official docs do not support
+ - For each related dialect in the same parser family, decide whether the same root cause exists, whether the PR also fixes it, and whether extra review feedback is required; do not silently treat unreviewed related dialects as safe
+ - Check ShardingSphere docs, examples, and release-note expectations for parser behavior changes; if docs and parser behavior diverge, require correction or explicit explanation
- If shared code is touched, build a blast-radius list of affected dialects/features and review at least one non-target example against its documented semantics
- If config flags or temporary properties exist on the touched path, review both enabled and disabled states
5. Test adequacy:
- Is there a failing case first or reproducible steps?
- Are major branches, boundaries, and counterexamples covered?
- Are tests mapped one-to-one with fix points?
+ - For SQL parser family scans, check whether each related dialect with the same root cause has direct validation or explicit evidence for non-applicability
- Distinguish fixture-assisted validation from production-path validation; if tests bypass the real assembly chain,
metadata loader, SPI discovery path, or routing path, state that gap explicitly and downgrade confidence
- If the PR adds multiple static metadata definitions, verify that regression tests cover the originally reported objects one-to-one; do not infer coverage from a single representative object unless the code path is truly identical and that equivalence is stated
@@ -146,9 +177,11 @@ When information gaps block mergeability, request at least:
- Is it only adding null checks/defaults/try-catch without fixing root cause?
- Does it introduce silent error swallowing or downgrade to wrong semantics?
- Are adjacent paths sharing the same root cause also validated?
+- If SQL parser is touched, were related trunk / branch dialects checked for the same root cause or divergence in shared / copied parser logic?
- Does the fix preserve the original precedence or lookup semantics for adjacent valid cases?
- Would a same-name or shadowing case take a different path after this patch?
- If the fix sits in shared code, would another dialect or feature now take a different path from its documented semantics?
+- Do the target database official docs and ShardingSphere docs/examples both support the resulting parser behavior, or is a documentation mismatch still open?
- Does the config-disabled or feature-flag-off path still behave correctly?
If the root-cause chain cannot be fully proven fixed, set `Merge Verdict: Not Mergeable`.
@@ -166,6 +199,8 @@ If the root-cause chain cannot be fully proven fixed, set `Merge Verdict: Not Me
- Feature-flag or disabled-path compatibility
- Functional degradation risk: old-scenario regression, boundary input behavior changes, error-code/exception semantic drift.
- Cross-dialect/shared-path risk: a target-specific fix changing generic behavior for other databases or features.
+- SQL parser family risk: the target dialect is fixed but branch dialects or copied parser variants still retain the same defect or become inconsistent.
+- Documentation risk: parser behavior changes without matching official-doc support or without aligning ShardingSphere docs/examples.
- Operational risk: config migration complexity, gray-release and rollback complexity.
- Supply-chain risk: vulnerabilities, licenses, transitive dependency changes from new deps.
@@ -176,6 +211,7 @@ Each review must declare:
- `Reviewed Scope`: files/modules actually reviewed this round.
- `Not Reviewed Scope`: unreviewed or only superficially reviewed areas.
- `Need Expert Review`: whether domain reviewers are required (security, concurrency, performance, protocol, etc.).
+- For SQL parser reviews, `Reviewed Scope` must explicitly name the target dialect, any related trunk / branch dialects checked, and the documentation pages / repo doc paths used to validate syntax behavior.
## Multi-Round Change Request Comparison Rules
diff --git a/parser/sql/engine/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/engine/mysql/visitor/statement/MySQLStatementVisitor.java b/parser/sql/engine/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/engine/mysql/visitor/statement/MySQLStatementVisitor.java
index 393899691b7..3aa8100efcb 100644
--- a/parser/sql/engine/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/engine/mysql/visitor/statement/MySQLStatementVisitor.java
+++ b/parser/sql/engine/dialect/mysql/src/main/java/org/apache/shardingsphere/sql/parser/engine/mysql/visitor/statement/MySQLStatementVisitor.java
@@ -887,8 +887,10 @@ public abstract class MySQLStatementVisitor extends MySQLStatementBaseVisitor<AS
private Collection<InsertValuesSegment> createRowConstructorList(final RowConstructorListContext ctx) {
Collection<InsertValuesSegment> result = new LinkedList<>();
- for (AssignmentValuesContext each : ctx.assignmentValues()) {
- result.add((InsertValuesSegment) visit(each));
+ for (int index = 0; index < ctx.assignmentValues().size(); index++) {
+ AssignmentValuesContext each = ctx.assignmentValues(index);
+ InsertValuesSegment insertValuesSegment = (InsertValuesSegment) visit(each);
+ result.add(new InsertValuesSegment(ctx.ROW(index).getSymbol().getStartIndex(), insertValuesSegment.getStopIndex(), insertValuesSegment.getValues()));
}
return result;
}
diff --git a/test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/asserts/segment/insert/InsertValuesClauseAssert.java b/test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/asserts/segment/insert/InsertValuesClauseAssert.java
index 3aa7cb7205c..26fae2258fc 100644
--- a/test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/asserts/segment/insert/InsertValuesClauseAssert.java
+++ b/test/it/parser/src/main/java/org/apache/shardingsphere/test/it/sql/parser/internal/asserts/segment/insert/InsertValuesClauseAssert.java
@@ -22,14 +22,15 @@ import lombok.NoArgsConstructor;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.assignment.InsertValuesSegment;
import org.apache.shardingsphere.sql.parser.statement.core.segment.dml.expr.ExpressionSegment;
import org.apache.shardingsphere.test.it.sql.parser.internal.asserts.SQLCaseAssertContext;
+import org.apache.shardingsphere.test.it.sql.parser.internal.asserts.segment.SQLSegmentAssert;
import org.apache.shardingsphere.test.it.sql.parser.internal.asserts.segment.assignment.AssignmentValueAssert;
import org.apache.shardingsphere.test.it.sql.parser.internal.cases.parser.jaxb.segment.impl.insert.ExpectedInsertValue;
import org.apache.shardingsphere.test.it.sql.parser.internal.cases.parser.jaxb.segment.impl.insert.ExpectedInsertValuesClause;
import java.util.Collection;
-import static org.hamcrest.Matchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.is;
/**
* Insert values clause assert.
@@ -54,6 +55,9 @@ public final class InsertValuesClauseAssert {
}
private static void assertInsertValues(final SQLCaseAssertContext assertContext, final InsertValuesSegment actual, final ExpectedInsertValue expected) {
+ if (isNeedAssertSQLSegment(expected)) {
+ SQLSegmentAssert.assertIs(assertContext, actual, expected);
+ }
assertThat(assertContext.getText("Insert assignment value size assertion error: "), actual.getValues().size(), is(expected.getAssignmentValues().size()));
int count = 0;
for (ExpressionSegment each : actual.getValues()) {
@@ -61,4 +65,8 @@ public final class InsertValuesClauseAssert {
count++;
}
}
+
+ private static boolean isNeedAssertSQLSegment(final ExpectedInsertValue expected) {
+ return expected.getStartIndex() > 0 || expected.getStopIndex() > 0 || null != expected.getLiteralStartIndex() || null != expected.getLiteralStopIndex();
+ }
}
diff --git a/test/it/parser/src/main/resources/case/dml/insert.xml b/test/it/parser/src/main/resources/case/dml/insert.xml
index 64314b42b4c..e1857bf29bf 100644
--- a/test/it/parser/src/main/resources/case/dml/insert.xml
+++ b/test/it/parser/src/main/resources/case/dml/insert.xml
@@ -1010,7 +1010,7 @@
<column name="status" start-index="40" stop-index="45" />
</columns>
<values>
- <value>
+ <value start-index="54" stop-index="65" literal-stop-index="74">
<assignment-value>
<parameter-marker-expression parameter-index="0" start-index="58" stop-index="58" />
<literal-expression value="1000" start-index="58" stop-index="61" />
@@ -1024,7 +1024,7 @@
<literal-expression value="init" start-index="68" stop-index="73" />
</assignment-value>
</value>
- <value>
+ <value start-index="68" stop-index="79" literal-start-index="77" literal-stop-index="97">
<assignment-value>
<parameter-marker-expression parameter-index="3" start-index="72" stop-index="72" />
<literal-expression value="1100" start-index="81" stop-index="84" />
diff --git a/test/it/parser/src/main/resources/case/dml/replace.xml b/test/it/parser/src/main/resources/case/dml/replace.xml
index 0cfc196f737..30fdf253128 100644
--- a/test/it/parser/src/main/resources/case/dml/replace.xml
+++ b/test/it/parser/src/main/resources/case/dml/replace.xml
@@ -496,7 +496,7 @@
<column name="status" start-index="41" stop-index="46" />
</columns>
<values>
- <value>
+ <value start-index="55" stop-index="66" literal-stop-index="75">
<assignment-value>
<parameter-marker-expression parameter-index="0" start-index="59" stop-index="59" />
<literal-expression value="1000" start-index="59" stop-index="62" />
@@ -510,7 +510,7 @@
<literal-expression value="init" start-index="69" stop-index="74" />
</assignment-value>
</value>
- <value>
+ <value start-index="69" stop-index="80" literal-start-index="78" literal-stop-index="98">
<assignment-value>
<parameter-marker-expression parameter-index="3" start-index="73" stop-index="73" />
<literal-expression value="1100" start-index="82" stop-index="85" />
diff --git a/test/it/parser/src/main/resources/case/dml/values.xml b/test/it/parser/src/main/resources/case/dml/values.xml
index 0ad116d74da..1a7ddd351b1 100644
--- a/test/it/parser/src/main/resources/case/dml/values.xml
+++ b/test/it/parser/src/main/resources/case/dml/values.xml
@@ -78,7 +78,7 @@
<expr>
<values-expression>
<values>
- <value>
+ <value start-index="7" stop-index="14">
<assignment-value>
<literal-expression value="1" start-index="11" stop-index="11" />
</assignment-value>
@@ -99,7 +99,7 @@
<expr>
<values-expression>
<values>
- <value>
+ <value start-index="7" stop-index="14">
<assignment-value>
<literal-expression value="1" start-index="11" stop-index="11" />
</assignment-value>
@@ -107,7 +107,7 @@
<literal-expression value="2" start-index="13" stop-index="13" />
</assignment-value>
</value>
- <value>
+ <value start-index="17" stop-index="24">
<assignment-value>
<literal-expression value="3" start-index="21" stop-index="21" />
</assignment-value>
diff --git a/test/it/parser/src/main/resources/sql/supported/dml/insert.xml b/test/it/parser/src/main/resources/sql/supported/dml/insert.xml
index 1e2092a1ab5..1832fc7ea40 100644
--- a/test/it/parser/src/main/resources/sql/supported/dml/insert.xml
+++ b/test/it/parser/src/main/resources/sql/supported/dml/insert.xml
@@ -46,7 +46,7 @@
<sql-case id="insert_set_without_generate_key_column" value="INSERT INTO t_order_item SET order_id = ?, user_id = ?, status = 'insert', creation_date='2017-08-08'" db-types="MySQL" />
<sql-case id="insert_set_with_row_alias" value="INSERT INTO t_order SET a = 1 AS new_alias(a)" db-types="MySQL" />
<sql-case id="insert_with_batch" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, ?), (?, ?, ?)" db-types="MySQL, SQLServer, PostgreSQL,openGauss,Doris" />
- <sql-case id="insert_with_batch_and_row_constructor" value="INSERT INTO t_order (order_id, user_id, status) VALUE ROW(?, ?, ?), ROW(?, ?, ?)" db-types="MySQL,Doris" />
+ <sql-case id="insert_with_batch_and_row_constructor" value="INSERT INTO t_order (order_id, user_id, status) VALUE ROW(?, ?, ?), ROW(?, ?, ?)" db-types="MySQL" />
<sql-case id="insert_with_batch_and_irregular_parameters" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, 1, 'insert'), (?, ?, ?)" db-types="MySQL, SQLServer, PostgreSQL,openGauss,Doris" />
<sql-case id="insert_with_batch_and_composite_expression" value="INSERT INTO t_order (order_id, user_id, status) VALUES (?, ?, SUBSTR(?, 1)), (?, ?, SUBSTR(?, 1))" db-types="H2,MySQL" />
<sql-case id="insert_with_batch_and_with_generate_key_column" value="INSERT INTO t_order_item(item_id, order_id, user_id, status, creation_date) values (?, ?, ?, 'insert', '2017-08-08'), (?, ?, ?, 'insert', '2017-08-08')" db-types="MySQL, SQLServer, PostgreSQL,openGauss,Doris" />
diff --git a/test/it/parser/src/main/resources/sql/supported/dml/replace.xml b/test/it/parser/src/main/resources/sql/supported/dml/replace.xml
index 045d03f0897..4f14cb18bf2 100644
--- a/test/it/parser/src/main/resources/sql/supported/dml/replace.xml
+++ b/test/it/parser/src/main/resources/sql/supported/dml/replace.xml
@@ -34,7 +34,7 @@
<sql-case id="replace_without_columns_and_without_generate_key_column" value="REPLACE INTO t_order_item values(?, ?, 'replace', '2017-08-08')" db-types="MySQL" />
<sql-case id="replace_set_without_generate_key_column" value="REPLACE INTO t_order_item SET order_id = ?, user_id = ?, status = 'replace', creation_date='2017-08-08'" db-types="MySQL" />
<sql-case id="replace_with_batch" value="REPLACE INTO t_order (order_id, user_id, status) VALUES (?, ?, ?), (?, ?, ?)" db-types="MySQL,Doris" />
- <sql-case id="replace_with_batch_and_row_constructor" value="REPLACE INTO t_order (order_id, user_id, status) VALUE ROW(?, ?, ?), ROW(?, ?, ?)" db-types="MySQL,Doris" />
+ <sql-case id="replace_with_batch_and_row_constructor" value="REPLACE INTO t_order (order_id, user_id, status) VALUE ROW(?, ?, ?), ROW(?, ?, ?)" db-types="MySQL" />
<sql-case id="replace_with_batch_and_irregular_parameters" value="REPLACE INTO t_order (order_id, user_id, status) VALUES (?, 1, 'replace'), (?, ?, ?)" db-types="MySQL,Doris" />
<sql-case id="replace_with_batch_and_with_generate_key_column" value="REPLACE INTO t_order_item(item_id, order_id, user_id, status, creation_date) values (?, ?, ?, 'replace', '2017-08-08'), (?, ?, ?, 'replace', '2017-08-08')" db-types="MySQL" />
<sql-case id="replace_with_batch_and_without_generate_key_column" value="REPLACE INTO t_order_item(order_id, user_id, status, creation_date) values (?, ?, 'replace', '2017-08-08'), (?, ?, 'replace', '2017-08-08')" db-types="MySQL" />
diff --git a/test/it/parser/src/main/resources/sql/supported/dml/values.xml b/test/it/parser/src/main/resources/sql/supported/dml/values.xml
index 86398fcca9c..9c14bde88b8 100644
--- a/test/it/parser/src/main/resources/sql/supported/dml/values.xml
+++ b/test/it/parser/src/main/resources/sql/supported/dml/values.xml
@@ -18,8 +18,8 @@
<sql-cases>
<sql-case id="values_with_regexp_replace" value="SELECT REGEXP_REPLACE(e, 'pattern', 'xyz')FROM (VALUES ROW('Find pattern'), ROW(NULL), ROW('Find pattern')) AS v(e)" db-types="MySQL" />
- <sql-case id="values_with_row" value="values ROW(1,2)" db-types="MySQL,Presto,Doris,Hive" />
- <sql-case id="values_row_two_rows_mysql" value="VALUES ROW(1,2), ROW(3,4)" db-types="MySQL,Doris" />
+ <sql-case id="values_with_row" value="values ROW(1,2)" db-types="MySQL" />
+ <sql-case id="values_row_two_rows_mysql" value="VALUES ROW(1,2), ROW(3,4)" db-types="MySQL" />
<sql-case id="values_with_order_limit" value="VALUES ROW(1,-2,3), ROW(5,7,9), ROW(4,6,8) ORDER BY column_1 desc , column_0 desc limit 10" db-types="MySQL" />
<sql-case id="values_with_select" value="values row((select 1), 2)" db-types="MySQL" />
</sql-cases>