terrymanu commented on PR #40176: URL: https://github.com/apache/shardingsphere/pull/40176#issuecomment-6210626595
### Result Review Result: Mergeable No blocking issues introduced by this change were found. This conclusion covers code correctness only. ### Evidence - The visitor processes both operands in source order and preserves both ESCAPE expressions, fixing missing parameter markers. Existing IN, BETWEEN, and LIKE paths remain unchanged. - Reviewed the eight placeholder/literal combinations generated by the four new cases, including parameter indices, operators, and character ranges. The syntax matches the [official Firebird 5 reference](https://www.firebirdsql.org/docs/chunk/en/refdocs/fblangref50/fblangref50-commons-predicates.html). - Production code and the test framework compiled successfully. Parser IT reached the 60-second limit during JAXB initialization, exiting with code 124 without assertion results. - Pre-existing limitation: Proxy does not traverse ListExpression, so ESCAPE predicates combined with parameters of other types can still produce incorrectly ordered type descriptions. Existing LIKE predicates are also affected. See the [current consumer](https://github.com/apache/shardingsphere/blob/da987d4add8249056743030f1a94887121c288d8/proxy/frontend/dialect/firebird/src/main/java/org/apache/shardingsphere/proxy/frontend/firebird/command/query/statement/prepare/FirebirdPrepareStatementCommandExecutor.java#L395-L405). ### Coverage - Candidate: latest public PR head `da987d4add8249056743030f1a94887121c288d8`; base `7df152f7eef8`; merge-base `3f85f839f789`. Remote state was rechecked. No local patches were included. - All three changed files were reviewed: [FirebirdStatementVisitor.java](https://github.com/apache/shardingsphere/blob/da987d4add8249056743030f1a94887121c288d8/parser/sql/engine/dialect/firebird/src/main/java/org/apache/shardingsphere/sql/parser/engine/firebird/visitor/statement/FirebirdStatementVisitor.java#L331-L394), [case/dml/select.xml](https://github.com/apache/shardingsphere/blob/da987d4add8249056743030f1a94887121c288d8/test/it/parser/src/main/resources/case/dml/select.xml#L15460-L15576), and [sql/supported/dml/select.xml](https://github.com/apache/shardingsphere/blob/da987d4add8249056743030f1a94887121c288d8/test/it/parser/src/main/resources/sql/supported/dml/select.xml#L588-L591). - The review used the PR description, existing Firebird grammar, README support statement, and CODE_OF_CONDUCT. The change repairs parameter registration for accepted syntax without adding public contracts or dialect commitments. Behavior clusters covered predicate ASTs, parameter ordering, and Parser IT baselines. Shared and trunk/branch dialect logic was unchanged. - Root-cause, contract-impact, and test/runtime discovery passes, followed by convergence review, were completed with no unresolved blocking evidence. - Local Spotless and Checkstyle checks both passed with exit code 0. Selection evidence confirmed coverage of the sole changed Java file. Command: `./mvnw -B -pl parser/sql/engine/dialect/firebird -DspotlessFiles="$JAVA_REGEX" -Dcheckstyle.includes="$JAVA_SOURCE_PATH" -Dcheckstyle.includeResources=false -Dcheckstyle.includeTestResources=false spotless:check checkstyle:check -Pcheck -T1` The variables respectively identify the file’s exact absolute-path regex in the candidate snapshot and its source-root-relative path. - Both XML files were Not Applicable to these checks: Spotless does not configure general XML formatting, and Checkstyle’s default resource filter selects properties files only. - Compilation scope: `parser/sql/engine/dialect/firebird,test/it/parser`; offline `test-compile` exited with code 0. In the same scope, `surefire:test -Dtest=org.apache.shardingsphere.test.it.sql.parser.firebird.InternalFirebirdParserIT -Dsurefire.failIfNoSpecifiedTests=false` exited with code 124. No real Firebird/Proxy execution was performed. - CI was not queried. The style results above came from local checks against the latest candidate. -- 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]
