You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "schizophrenicmaniac (via GitHub)" <gi...@apache.org> on 2026/10/20 22:12:36 UTC
[PR] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
schizophrenicmaniac opened a new pull request, #40309:
URL: https://github.com/apache/shardingsphere/pull/40309
Fixes #40308.
Changes proposed in this pull request:
- Add `visitInsertSelectClause` to `FirebirdDMLStatementVisitor` so `INSERT ... SELECT` builds its insert columns and the SELECT subquery (with its parameter markers) instead of failing with an NPE on the missing `insertValuesClause`.
- In `FirebirdPrepareStatementCommandExecutor`, describe the parameters of `INSERT ... SELECT` from the query, in parameter marker order. `col BETWEEN ? AND ?` now gets the column type too. Parameters that can't be matched to a column still fall back to VARCHAR, now at their own position instead of after the matched ones.
- Answer `isc_info_sql_stmt_select` for `INSERT ... SELECT ... RETURNING`, so the client gets a cursor with all inserted rows, as the Firebird server does. `INSERT ... VALUES ... RETURNING` keeps `exec_procedure`.
- Expand `RETURNING *` of an INSERT over the target table only, not over the tables of the query.
- Parser IT: enable Firebird for the existing `insert_select_with_all_columns` and `insert_select_without_columns` cases, and add `insert_select_with_returning_firebird`. Add unit tests for the parameter description and the `RETURNING *` cursor case.
---
Before submitting this PR, I confirm that:
- [x] My code follows the [code of conduct](https://shardingsphere.apache.org/community/en/involved/conduct/code/) of this project.
- [x] I have reviewed my changes.
- [ ] I have (or in comment I request) added corresponding labels for the pull request.
- [x] I have passed the required local checks: `./mvnw clean install -B -T1C -Pcheck`.
- [ ] I have updated the relevant documentation, if applicable.
- [x] I have added or updated unit tests for my changes, if applicable.
- [ ] I have updated the Release Notes of the current development version, if applicable. For more details, see [Update Release Note](https://shardingsphere.apache.org/community/en/involved/contribute/contributor/).
AI disclosure: drafted with Claude Code (Claude Opus 5.5); files: FirebirdDMLStatementVisitor.java, FirebirdPrepareStatementCommandExecutor.java, FirebirdPrepareStatementCommandExecutorTest.java, insert.xml, insert.xml.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6321376706
@terrymanu Merged master and resolved the conflicts. Parameter types are still mapped by marker index, now alongside the UNION where-segment handling from master, and the Firebird tests pass.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6606044143
@terrymanu Thanks, both are fixed in the latest commit.
1. Parameters in `col IN (?, ...)` are now matched to the column the same way BETWEEN bounds are. For an INTEGER key they're described as INTEGER, so a client using setLong(10) sends 10 instead of "10" and gets the same HASH_MOD route as INSERT ... VALUES. For a NUMERIC/DECIMAL column their positions are recorded, so the execute path converts them back to numbers. assertDescribeInsertSelectInParameters covers `INSERT INTO foo_tbl (id) SELECT id + 2 FROM foo_tbl WHERE id IN (?, ?)`.
2. Added @Nullable to columnNames in createInsertColumnsSegment.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6475142646
@terrymanu Thanks, both are fixed in the latest commit.
1. Parameters matched to a DECIMAL/NUMERIC or TIME WITH TIME ZONE column (BETWEEN bounds, comparisons and UPDATE SET) are now described as VARCHAR(32765), the same fallback the CAST path already uses for those types. The value then reaches the backend as text, so 12.34 stays 12.34 and there's no TIME_TZ value for the execute side to decode. assertDescribeExactNumericAndTimeWithTimeZoneColumnParametersAsVarchar covers an INSERT ... SELECT with `amount BETWEEN ? AND ? AND start_time = ?`.
2. Character CAST parameters are now described with a byte length of the declared characters times 4, the most any Firebird character set needs per character (UTF8), capped at 32765. So CAST(? AS VARCHAR(5) CHARACTER SET UTF8) comes back with LENGTH 20, enough for five 3-byte Chinese characters. With a single-byte connection charset an over-long value is then rejected by the backend CAST rather than by Jaybird. assertDescribeCharacterCastParametersWithMultibyteLength checks VARCHAR(5) UTF8, CHAR and the 32765 cap.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6460886721
@terrymanu Thanks, pushed fixes for all five.
1. I didn't describe DECIMAL/NUMERIC with a scale, because the execute side can't use it yet: FirebirdBlrRowMetadata skips the scale byte in the client's BLR, and the INT64/INT128 values are read as plain integers. If prepare said INT64 with scale -2, Jaybird would send 12.34 as 1234 and the proxy would bind 1234. So a CAST to DECIMAL or NUMERIC is now described as VARCHAR(32765). The client sends the value as text and the backend CAST applies the declared precision and scale, so 12.34 and values over 18 digits go through unchanged. Proper scaled numeric binding would mean changing the protocol execute path, which I'd rather keep out of this PR.
2. FLOAT(p) with p > 24 is now described as DOUBLE PRECISION. FLOAT and FLOAT(24) stay single precision.
3. TIME WITH TIME ZONE also falls back to VARCHAR now, since there's no TIME_TZ decoder on the execute side. Unrecognised type names now always use the 32765 length rather than the declared precision.
4. Renamed the ifPresent parameters to selectStatement and combine, and the local to params.
5. Fixed both wrapping positions.
assertDescribeExactNumericAndTimeWithTimeZoneCastParametersAsVarchar covers DECIMAL(10,2), NUMERIC(38,4) and TIME WITH TIME ZONE with LENGTH, and assertDescribeFloatCastParametersByPrecision covers FLOAT(24) and FLOAT(53).
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6450234332
@terrymanu Thanks, all four are fixed in the latest commit.
1. A parameter inside CAST(? AS type) is now described with the type and length it's cast to, so CAST(? AS VARCHAR(20)) comes back as VARCHAR(20) instead of VARCHAR(32765). The Firebird CAST visitor was dropping the parameter marker from the function arguments, so it now keeps it, and the prepare executor picks up CAST types from the select list (including UNION branches), WHERE and UPDATE SET. Unknown type names (domains, for example) still fall back to VARCHAR. assertDescribeInsertSelectCastParameters requests LENGTH and checks INTEGER and VARCHAR with length 20.
2. Moved processUnknownParameter after the helpers called before it.
3. Put the return in isShardingInsertWithReturning back on one line.
4. Added @HighFrequencyInvocation to ShardingDMLResultMerger.merge.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6338782910
@terrymanu Thanks for the review, all three are fixed in the latest commit.
1. Parameters that can't be matched to a column are now described as VARCHAR with a length of 32765 (the Firebird VARCHAR maximum), so the LENGTH item is always written. The INSERT ... SELECT test now requests LENGTH and checks it.
2. For INSERT ... SELECT, the where segments now come from the inner SelectStatementContext through the same UNION traversal used for a plain SELECT. I added a three-branch INSERT ... SELECT test that expects INTEGER/BLOB/INTEGER.
3. Removed both line comments.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6594780639
@terrymanu Thanks, all four are addressed in the latest commit.
1. Parameters matched to a NUMERIC/DECIMAL column are still described as VARCHAR, so the client sends the exact text and 12.34 doesn't get rounded. The prepared statement now records those parameter positions, and the execute and batch execute paths convert the text back to a number before routing. Integral values become Long, the same type INSERT ... VALUES binds them with through INT64, so a client using setLong(10) hits the same shard for writes and reads. Fractional values and values outside the long range become BigDecimal, and anything that isn't a number is left for the backend to reject. FirebirdExecuteStatementCommandExecutorTest checks that "10" reaches the query context as 10L, FirebirdServerPreparedStatementTest covers the conversion cases, and the prepare test checks which positions get recorded. TIME WITH TIME ZONE columns still go through as text since they're not converted.
2. CHAR casts are now capped at 32767 bytes, VARCHAR stays at 32765. CHAR(32767) CHARACTER SET ISO8859_1 is covered in assertDescribeCharacterCastParametersWithMultibyteLength.
3. Only `BLOB` and `BLOB ...` (sub type, segment size, abbreviated attributes) are treated as BLOB now, so CAST(? AS BLOB_NUMBER) falls back to VARCHAR like other unresolved domains. Added assertDescribeBlobAndDomainCastParameters.
4. Renamed the Optional.map parameter to `optional`.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6361200219
@terrymanu Merged master again and resolved the conflicts. The PR's changes now sit on top of the Firebird parser and frontend simplifications from master.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6341001812
Please resolve the conflict
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6312737924
Please resolve conflicts
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub)" <gi...@apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6384356040
@terrymanu Thanks, both are fixed in the latest commit.
1. ShardingResultMergerEngine now uses a new ShardingDMLResultMerger for an INSERT with RETURNING into a sharding table. It iterates over the results of every route instead of keeping only the first one. I limited it to sharding tables because an INSERT into a broadcast table runs on every data source and would otherwise return each row several times. ShardingDMLResultMergerTest merges two backend results and checks that the rows from both come back, and ShardingResultMergerEngineTest covers picking the new merger.
2. Moved putAffectedColumn to right after findAffectedColumns.
--
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] Fix INSERT ... SELECT prepare in Firebird Proxy [shardingsphere]
Posted by "schizophrenicmaniac (via GitHub) via notifications" <no...@shardingsphere.apache.org>.
schizophrenicmaniac commented on PR #40309:
URL: https://github.com/apache/shardingsphere/pull/40309#issuecomment-6772211021
@terrymanu Thanks, both are fixed in the latest commit.
1. Comparisons now match the column on either side, so `? = id` is described with the column type just like `id = ?`. For an INTEGER key the parameter comes back as INTEGER, so setLong(10) is sent as 10 and takes the same HASH_MOD route as INSERT ... VALUES. For a NUMERIC/DECIMAL column, the position is recorded and the execute path converts the value back to a number before routing. assertDescribeInsertSelectReversedComparisonParameters covers `INSERT INTO foo_tbl (id) SELECT id + 2 FROM bar_tbl WHERE ? = id AND ? < amount`. It checks INTEGER for the first parameter, VARCHAR(32765) for the second, and that position 1 is recorded for conversion. Routing itself is unchanged, so the existing execute test that checks "10" reaching the query context as 10L covers the conversion side.
2. Switched to `Arrays.asList("10", null)` directly.
--
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