Fix sharding routing dropping ?::int4 casts on INSERT VALUES sharding key

[Phixsura]

Fixes #23764.

Changes proposed in this pull request:

• Unwrap nested TypeCastExpression to its underlying expression at the top of InsertClauseShardingConditionEngine.createShardingCondition. The existing instanceof SimpleExpressionSegment / CommonExpressionSegment / isNowExpression chain has no branch for TypeCastExpression (a ComplexExpressionSegment), so a PostgreSQL ?::int4 cast on the sharding key was silently dropped and the INSERT broadcast to every shard.
• InsertClauseShardingConditionEngineTest: add three regression cases covering a TypeCast over a parameter marker, over a literal, and nested casts (e.g., ?::int4::text).
• e2e-dml-insert.xml: enable the ?::int4 INSERT case that was previously commented out with TODO Fix #23764.

ConditionValue / WhereClauseShardingConditionEngine have the same shape of gap for WHERE-clause type casts, but the reported scenario only exercises INSERT VALUES; sending that as a separate follow-up.

---

Before committing this PR, I'm sure that I have checked the following options:

• My code follows the code of conduct of this project.
• I have self-reviewed the commit code.
• I have (or in comment I request) added corresponding labels for the pull request.
• I have passed maven check locally for the affected modules (spotless, checkstyle, unit tests).
• I have made corresponding changes to the documentation.
• I have added corresponding unit tests for my changes.
• I have updated the Release Notes of the current development version. For more details, see Update Release Note

[Phixsura]

Workflow runs are still pending committer approval. Could a maintainer kindly approve them?
Local checks (spotless, checkstyle, unit + IT, JDK 21 reactor build) are all green.

[terrymanu]

Decision

Merge Verdict: Not Mergeable
Reviewed Scope: PR #38698 latest head 6e2955a, four changed files, issue #23764, CI/check-runs, PostgreSQL/openGauss TypeCast visitor paths, INSERT sharding condition path.
Not Reviewed Scope: full local Maven execution, exhaustive sharding algorithm behavior, live openGauss SQL E2E.
Need Expert Review: sharding routing and PostgreSQL/openGauss cast semantics.
Positive Feedback

The PR is aimed at the right root-cause area: TypeCastExpression hides the underlying parameter marker from InsertClauseShardingConditionEngine, which matches the issue’s empty sharding-condition symptom. Re-enabling the E2E INSERT case is also the right direction.

Major Issues

P1: Type-cast semantics are not preserved.
InsertClauseShardingConditionEngine.java:137-159 unwraps every TypeCastExpression and routes using the raw inner parameter/literal value. PostgreSQL documents expression::type as a run-time type conversion, so ?::int4 is not always semantically equal to the original Java parameter. A counterexample is a string parameter "1" cast to int4: PostgreSQL can evaluate it as integer, while routing still sees String. This can route to the wrong shard. Please either convert supported cast targets before creating ListShardingConditionValue, or narrow the fix to only cases whose semantics are proven safe, with tests for string/literal cast inputs.

P1: Broad unwrapping introduces a ClassCastException path.
SimpleExpressionSegment also includes SubqueryExpressionSegment, not only literal/parameter markers. After this PR, a PostgreSQL value like ((SELECT 1)::int4) can unwrap to a SubqueryExpressionSegment, enter the SimpleExpressionSegment branch, and then fail in getShardingValue when it casts non-parameter values to LiteralExpressionSegment at InsertClauseShardingConditionEngine.java:171-174. Please accept only ParameterMarkerExpressionSegment / LiteralExpressionSegment after unwrapping, or handle unsupported simple expressions without crashing, and add a counterexample test.

P2: Validation is still too narrow for the changed shared routing path.
The path is used by both JDBC and Proxy INSERT routing. CI reports 82 successful check-runs, including PostgreSQL SQL E2E, Spotless, Checkstyle, and License, but I did not find SQL E2E check-runs for openGauss even though the test case declares PostgreSQL,openGauss. The new unit tests also assert size/parameter index but not the actual routed sharding values. Please add direct assertions for extracted values and provide openGauss SQL E2E evidence or narrow the declared scope.

Newly Introduced Issues

The PR changes previously ignored casted non-literal expressions into a possible routing-time exception path. This should be fixed or rolled back before merge.
Next Steps

Restrict unwrapTypeCast handling to supported inner expression types, or add safe cast-aware conversion for known PostgreSQL/openGauss types such as int4.
Add regression tests for:
?::int4 with a string parameter.
'1'::int4 or equivalent literal cast with actual value assertion.
TypeCastExpression wrapping SubqueryExpressionSegment does not crash.
PostgreSQL and openGauss evidence for the enabled E2E case.
Re-run PR checks after changes.

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: Latest PR head 62d1e1d8263fc40b005273227e63e6b5f8b01974; RELEASE-NOTES.md, features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/InsertClauseShardingConditionEngine.java, features/sharding/core/src/test/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/InsertClauseShardingConditionEngineTest.java, test/e2e/sql/src/test/resources/cases/dml/e2e-dml-insert.xml, related ConditionValue / WhereClauseShardingConditionEngine, PostgreSQL/openGauss TypeCastExpression producers, and official PostgreSQL/openGauss cast semantics docs.
• Not Reviewed Scope: Full local Maven build, Docker E2E execution, non-sharding modules, and full CI logs beyond the GitHub check-run summary.
• Need Expert Review: Yes, sharding routing plus PostgreSQL/openGauss cast semantics after the routing behavior is corrected.

Positive Feedback

• This targets the real INSERT trigger: TypeCastExpression hides the underlying parameter marker from insert sharding condition extraction, and the PR adds useful regression coverage for the reported PostgreSQL integer-marker path.

Major Issues

• [P1] Cast routing uses the raw Java value instead of the SQL cast result (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/InsertClauseShardingConditionEngine.java:162)

  • Symptom: unwrapTypeCastForRouting returns the innermost marker/literal at line 223, then getShardingValue reads the original parameter/literal at lines 259-262. The value instanceof Number branch at lines 235-236 treats every numeric target cast as safe without applying the target cast.
  • Risk: PostgreSQL documents expression::type as a runtime conversion, and its numeric docs show conversions can round values. For example, ?::int4 bound as a decimal value can be routed using the uncast Java value while PostgreSQL stores the cast integer result, causing a wrong single-shard route.
  • Action: Please either compute the exact casted value for a small documented set of supported PostgreSQL/openGauss casts before creating ListShardingConditionValue, or restrict unwrapping to exact value-preserving cases with explicit tests for lossy numeric conversions and overflow.

• [P1] Valid casted string/literal cases are still codified as unrouteable (features/sharding/core/src/test/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/InsertClauseShardingConditionEngineTest.java:252)

  • Symptom: The new tests expect no sharding condition for ?::int4 when the bound value is "1" and for '1'::int4 at lines 288-295. PostgreSQL documents that valid string literals can be assigned a target type by cast, and openGauss documents cast(x as y) as converting x into y.
  • Risk: These are adjacent valid inputs for the same root cause: the sharding key is present but wrapped in a cast. Leaving the condition empty reintroduces the original multi-node INSERT failure instead of routing by the database-visible value.
  • Action: Please add regression tests that validate supported string/literal casts route by the cast result, not by the raw Java/string value, and avoid asserting valid database casts as "does not route" unless ShardingSphere explicitly rejects them before routing with a documented error.

• [P1] openGauss and WHERE-clause blast radius remain unresolved (test/e2e/sql/src/test/resources/cases/dml/e2e-dml-insert.xml:34)

  • Symptom: The original issue and parser fixtures cover PostgreSQL and openGauss, and both PostgreSQL/openGauss visitors create TypeCastExpression; however the enabled E2E case is narrowed to PostgreSQL only. The PR body also acknowledges the same TypeCast gap in ConditionValue / WhereClauseShardingConditionEngine, where features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/generator/ConditionValue.java:47 returns null for non-marker/non-literal expressions.
  • Risk: This shared sharding-condition path affects Proxy/JDBC routing. Without openGauss validation and a decision for WHERE predicates such as order_id = ?::int4, the same hidden-parameter root cause remains open for a related dialect and adjacent DML/DQL paths.
  • Action: Please restore or add openGauss validation, or provide direct evidence that openGauss is not applicable. Please also fix the shared extraction path or add scoped validation/issue linkage showing why WHERE-clause cast routing is intentionally out of scope without changing this PR's behavior.

Next Steps

• Please replace the category-only cast check with cast-result-aware routing, or narrow the supported cases to exact value-preserving casts only.
• Please add counterexample tests for valid string/literal casts, lossy numeric casts, and overflow/error cases.
• Please restore openGauss coverage or provide direct non-applicability evidence.
• Please cover the adjacent WHERE-clause TypeCast path or clearly isolate it with a tracked follow-up and no behavior regression in this PR.
• Please clean the added whitespace reported by git diff --check, then rerun the scoped unit tests, relevant SQL E2E case, Spotless, and Checkstyle.

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: Latest PR head 3e59e09f0e32041bdfb8c91570230ce57f298e17; #23764; sharding core INSERT VALUES and WHERE-clause condition extraction; PostgreSQL/openGauss TypeCastExpression paths; RELEASE-NOTES.md; test/e2e/sql/src/test/resources/cases/dml/e2e-dml-insert.xml; PostgreSQL official docs for type casts, numeric types, and character types.
• Not Reviewed Scope: Full Maven build, Docker E2E execution, CI/check-runs, exhaustive PostgreSQL/openGauss cast catalog behavior, and non-sharding modules.
• Need Expert Review: Yes, sharding routing plus PostgreSQL/openGauss cast semantics after the blockers below are fixed.

Positive Feedback

• The PR is moving in the right direction: it now targets the real TypeCastExpression trigger, restores the openGauss E2E declaration, adds subquery non-crash coverage, and attempts to cover the adjacent WHERE-clause path.

Major Issues

• [P1] Numeric-to-integer casts use the wrong tie-breaking rule (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/PostgreSQLCastEvaluator.java:41)

  • Symptom: The evaluator documents and implements ROUND_HALF_EVEN for integer casts at lines 134-146, and the test expects 2.5 to cast to 2 at features/sharding/core/src/test/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/PostgreSQLCastEvaluatorTest.java:99.
  • Risk: PostgreSQL documents numeric tie rounding away from zero, with 2.5 rounding to 3. A sharding key like 2.5::int4 can therefore route to shard 2 while PostgreSQL evaluates the cast result as 3.
  • Action: Please align integer cast evaluation with the documented database-visible result, or reject these lossy numeric casts instead of routing them. Please add counterexamples for 2.5::int4 and -2.5::int4 through both INSERT and WHERE routing paths.

• [P1] Cast target modifiers are stripped before routing (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/PostgreSQLCastEvaluator.java:89)

  • Symptom: normalize removes everything after (, so varchar(1), char(2), and numeric(3,1) are treated as plain varchar, char, and numeric. The text conversion then returns the raw string at line 170, and numeric conversion returns the raw BigDecimal at line 149.
  • Risk: PostgreSQL documents explicit character varying(n) / character(n) casts as truncating over-length values, and numeric(p,s) as rounding to the declared scale with precision overflow checks. A sharding key such as ?::varchar(1) with value "ab" should route by "a", but this PR routes by "ab".
  • Action: Please either parse and apply supported type modifiers exactly, or treat modifier-bearing targets as unsupported so routing does not use a value different from the database-visible cast result. Please add counterexample tests for text and numeric typmods.

• [P2] WHERE-clause validation still bypasses the production extraction path (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/generator/ConditionValue.java:45)

  • Symptom: The WHERE fix is tested mainly by directly constructing ConditionValue, but production WHERE routing reaches it through compare / IN / BETWEEN generators and WhereClauseShardingConditionEngine.
  • Risk: The previous WHERE-clause blast-radius concern is only partially closed; generator merging, parameter marker propagation, and unsupported-cast fallback can still regress outside the direct fixture path.
  • Action: After fixing cast semantics, please add production-path tests for WHERE order_id = ?::int4, IN (?::int4), and BETWEEN ?::int4 AND ?::int4, including at least one unsupported or failing cast case.

Newly Introduced Issues

• The latest revision introduces PostgreSQLCastEvaluator, which is a new shared routing behavior surface. The numeric rounding and typmod issues above are newly introduced semantic divergences from PostgreSQL cast behavior.

Next Steps

• Please replace the current Java approximation with documented cast-result-aware handling, or narrow support to exact safe cases needed for #23764.
• Please add one-to-one regression tests for the counterexamples above in INSERT and WHERE production paths.
• Please provide openGauss evidence for the same supported cast set, or explicitly narrow the supported semantics to PostgreSQL until openGauss behavior is proven.
• Please rerun scoped unit tests with -am, then Spotless and Checkstyle according to the repo gates.

Multi-Round Comparison

• Fixed: The subquery cast crash path is covered by the new INSERT test.
• Fixed: Basic string/literal ?::int4 and '1'::int4 cases now route by a casted value.
• Partially fixed: Cast semantics are no longer raw-value-only, but numeric rounding and target modifiers still do not match PostgreSQL behavior.
• Partially fixed: openGauss is restored in the E2E case and WHERE support was added, but production-path WHERE validation and openGauss semantic evidence are still incomplete.

[Phixsura]

Pushed 8c938b29 on top of current master.

For the numeric-to-int rounding: switched castToShort / castToInteger / castToLong from HALF_EVEN to HALF_UP, so 2.5::int4 is 3 and -2.5::int4 is -3. Pinned in PostgreSQLCastEvaluatorTest, in ConditionValueTest (cast entry), and in InsertClauseShardingConditionEngineTest (INSERT routing). Flipping it back to HALF_EVEN reds both the 2.5 and -2.5 cases.

For typmod: went with the "treat modifier-bearing targets as unsupported" option. normalize() no longer strips the parenthesised part, so varchar(1), char(2), numeric(3,1) and the like fall through to OTHER and evaluate returns Optional.empty(), which leaves the cast in place and routes broadcast. Negative cases live in the evaluator unit test plus the INSERT and WHERE paths. Restoring the paren-strip in normalize() reds all four typmod cases.

For the WHERE production path: added four cases in WhereClauseShardingConditionEngineTest that drive createShardingConditions through ExpressionExtractor.extractAndPredicates and ConditionValueGeneratorFactory.generate:

• col = ?::int4 with String "42" returns [42] (Integer, not the raw String)
• col IN (?::int4) returns [42]
• col BETWEEN ?::int4 AND ?::int4 with "1" / "100" returns the range [1, 100]
• col = ?::numeric(3,1) returns an empty condition list

./mvnw test -pl features/sharding/core is 921/921; spotless and checkstyle clean.

On openGauss: the evaluator itself is dialect-agnostic Java semantics, so PG and openGauss take the same path through it. I did not add a separate openGauss E2E this round; happy to add one if you want it.

[terrymanu]

Decision

• Merge Verdict: Not Mergeable
• Reviewed Scope: Latest PR head 8c938b294dd4e700dc34ffc3642e3533336ba6f6; local merge-base a9ac265ad4adcc247a8af27f1febca026e6cc8d4; GitHub /pulls/38698/files matched the local triple-dot file list exactly: 9 files in features/sharding/core, test/e2e/sql, and RELEASE-NOTES.md. Reviewed PostgreSQL/openGauss TypeCastExpression routing through INSERT VALUES and WHERE compare/IN/BETWEEN paths. Checked related parser producers: PostgreSQL and openGauss produce TypeCastExpression; MySQL/MariaDB/Doris were not affected by this expression type. Used PostgreSQL official docs for type casts, numeric types, character types, boolean type, and PostgreSQL source pg_cast.dat / float.c; used openGauss type-conversion docs for the openGauss cast baseline.
• Not Reviewed Scope: Full local Maven build, Docker SQL E2E execution, live PostgreSQL/openGauss verification, exhaustive cast catalog behavior, and GitHub Actions/check-runs.
• Need Expert Review: Yes, sharding routing plus PostgreSQL/openGauss cast semantics after the blockers below are fixed.

Positive Feedback

• The PR now targets the real root-cause path: TypeCastExpression hides the sharding-key marker/literal from condition extraction.
• Several previous blockers are addressed in the right direction: raw-value routing for basic ?::int4, subquery non-crash behavior, typmod fallback, BigDecimal numeric tie rounding, WHERE production-path tests, and restored PostgreSQL/openGauss E2E declaration.

Major Issues

• [P1] char / character without typmod routes by the wrong value (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/PostgreSQLCastEvaluator.java:122)

  • Symptom: CHAR and CHARACTER are categorized as TEXT, and castToText returns value.toString() at line 181. PostgreSQL documents character without a length specifier as equivalent to character(1), and explicit casts to character(n) truncate over-length values. So ?::char bound as "ab" should be routed by the database-visible cast result, not by "ab".
  • Risk: INSERT and WHERE both call this evaluator (InsertClauseShardingConditionEngine.java:188, ConditionValue.java:98). A sharding key cast such as ?::char can be routed to the shard for "ab" while PostgreSQL stores/compares the cast result as character(1), causing wrong-shard routing.
  • Action: Please either implement PostgreSQL/openGauss char / character no-typmod semantics exactly, or treat those targets as unsupported like char(2) until exact semantics are implemented. Please add INSERT and WHERE counterexample tests for ?::char or 'ab'::char.

• [P1] Float/double-to-integer casts use numeric tie rounding (features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/route/engine/condition/engine/PostgreSQLCastEvaluator.java:143)

  • Symptom: castToShort, castToInteger, and castToLong always convert through BigDecimal and RoundingMode.HALF_UP; toBigDecimal also converts Java Float / Double via value.toString() at lines 214-215. PostgreSQL separates numeric casts from real / double precision casts: the docs state floating tie rounding is commonly nearest-even, and PostgreSQL source maps float8 -> int4 / float4 -> int4 to cast functions that use rint().
  • Risk: For ?::int4 bound as Java Double 2.5, this evaluator routes by 3, while PostgreSQL’s float cast path can produce 2 on the normal nearest-even path. That can insert or query using the wrong shard for the database-visible value.
  • Action: Please distinguish source value type. For Java BigDecimal / numeric inputs, keep the PostgreSQL numeric rule; for Java Float / Double, either emulate PostgreSQL’s float cast behavior safely or return Optional.empty(). Please add evaluator plus INSERT/WHERE production-path tests for 2.5D and -2.5D.

Newly Introduced Issues

• The newly added evaluator broadens routing from the originally reported ?::int4 integer-marker path into more cast families. The two issues above are new wrong-value routing risks in that broadened surface.

Next Steps

• Fix or reject unsupported char / character no-length casts before routing.
• Fix or reject Java Float / Double to integer casts before routing.
• Add one-to-one regression tests for the two counterexamples in both INSERT VALUES and WHERE paths.
• Re-run scoped sharding-core tests with dependent modules, then the repository Spotless and Checkstyle gates.

Multi-Round Comparison

• Fixed: Basic ?::int4 parameter and literal casts now route by a casted value instead of the raw value.
• Fixed: The casted subquery path no longer crashes and now extracts no condition.
• Fixed: BigDecimal numeric tie rounding now follows PostgreSQL numeric round-away-from-zero behavior.
• Fixed: Modifier-bearing targets such as varchar(1) / numeric(3,1) are treated as unsupported instead of routing by an unmodified value.
• Fixed: WHERE compare, IN, BETWEEN, and unsupported-typmod cases now use the production condition extraction path.
• Newly introduced / still unresolved: char / character no-length casts and Java Float / Double integer casts can still route by a value different from PostgreSQL’s database-visible cast result.

[Phixsura]

Pushed fc325eed on top of current master.

For the char / character no-typmod cell (P1.A): castToText now splits the TEXT family into three sub-targets. text / varchar / bpchar keep identity, naked char / character truncate to the first Unicode codepoint via String#offsetByCodePoints, and name truncates to 63 UTF-8 bytes (preserving multi-byte boundaries). 'ab'::char therefore routes by 'a' and repeat('中', 70)::name routes by the 21-char prefix (63 bytes). Counterexamples in INSERT and WHERE production paths.

For the Java Float / Double to integer cell (P1.B): the evaluator was rebuilt around an explicit (source category, target category) dispatch instead of the previous toBigDecimal-flattened single path. Java BigDecimal / BigInteger route through numeric_int4 (PostgreSQL's numeric.c, round-away-from-zero, Java HALF_UP), while Java Float / Double route through dtoi4 / ftoi4 (PostgreSQL's float.c, rint(), Java Math#rint = HALF_EVEN). 2.5::numeric::int4 returns 3, 2.5::float8::int4 returns 2, matching PostgreSQL 16.

While restructuring the matrix I cross-checked the rest of pg_cast against PostgreSQL 16 via postgres:16-alpine for the full set of (source, target) cells and added counterexamples that PG itself rejects: bool::numeric, numeric::bool, float8::bool, '1.5'::int4, '1.0'::int4, '1e0'::int4 all return empty. Boolean::text correctly emits "true"/"false" for text / varchar / bpchar and "t"/"f" for name (the latter via PG's boolout path). Float/Double to numeric strips trailing zeros and rounds to 15 significant digits to match PostgreSQL's %.15g formatting.

openGauss verification: rather than spinning up a container I cross-referenced pg_cast.h from opengauss-mirror/openGauss-server. The integer / float / numeric / bpchar / namein cast function OIDs (1740 / 317 / 319 / 3192 / 19) match PostgreSQL byte-for-byte. The one substantive divergence is that openGauss adds bool ↔ numeric cast functions 6433 / 6434 that PostgreSQL does not have; the evaluator deliberately stays aligned with PostgreSQL and rejects those, which on openGauss is a conservative narrowing (broadcast instead of routing by 0 / 1). This is documented inline on the class and pinned by two explicit unit tests.

Scope check on the broadened routing surface: PostgreSQLCastEvaluator is only reachable through ConditionValue (WHERE / IN / BETWEEN / UPDATE / DELETE) and InsertClauseShardingConditionEngine (INSERT VALUES). TypeCastExpression itself is only emitted by the PostgreSQL and openGauss statement visitors; MySQL / Oracle / SQL Server CAST(...) syntax becomes a FunctionSegment and bypasses this evaluator entirely, so the broadened cast handling cannot leak into other dialects.

E2E: 4 additional e2e-dml-insert.xml cases drive the new cells through actual PostgreSQL and openGauss containers — ?::int4 with 2.5:double and 2.5:float (routes shard 2 via Float/Double HALF_EVEN), 2.5::numeric::int4 (routes shard 3 via numeric HALF_UP), 2.5::float8::int4 (routes shard 2). All reuse existing insert_for_order_2.xml / insert_for_order_3.xml. Both PG and openGauss must agree that the casted row lands in the expected shard for the assertion to pass.

./mvnw test -pl features/sharding/core is 1014/1014 (+93 new); spotless and checkstyle clean.

[terrymanu]

Please fix the CI

[terrymanu]

Please resolve conflicts first

[terrymanu]

Hi @Phixsura, thanks again for the continued updates on this PR.

The fix direction looks reasonable because it targets the TypeCastExpression path that hides PostgreSQL/openGauss casted sharding-key values from sharding condition extraction. The later revisions also addressed several previous cast-semantics review concerns.

However, this PR is currently not ready for review or merge because it has conflicts with the target branch, and the latest maintainer feedback asked to resolve conflicts first. Could you please rebase this PR and resolve the conflicts so we can continue the focused review?

To keep the review queue clear, we may close this PR soon due to inactivity if there is still no response or update.

Thanks again for your contribution.