Fix COUNT(*) handling for Firebird#37494
Merged
Merged
Conversation
terrymanu
requested changes
Dec 24, 2025
Member
terrymanu
left a comment
There was a problem hiding this comment.
- Good job adding the Firebird projection identifier extractor and tests—the implementation aligns with other dialects and is a step in the right direction.
- Blocking gap: the new SPI is not loaded at runtime. proxy/dialect/firebird/pom.xml and jdbc-dialect/firebird/pom.xml don’t include the Firebird binder module, and distribution/bom/pom.xml doesn’t expose it. Without these dependencies, the new extractor won’t be on the classpath and COUNT labels will still fall back to the default. Please wire the module into the proxy/JDBC dialect POMs and BOM.
- New logic for lowercasing function names and mapping COUNT to BIGINT lacks coverage. Please add a test (e.g., in FirebirdPrepareStatementCommandExecutorTest or a nearby test) that exercises describe handling/type resolution so regressions are caught.
- Once these gaps are fixed, this should be safe to merge. Keep going—you’re close.
Contributor
Author
Yes, you’re right. I initially built the project with these dependencies, but later cleaned them up and forgot to rebuild the project and verify that everything was still working correctly. Thanks for your comments! |
terrymanu
approved these changes
Dec 25, 2025
makssent
pushed a commit
to makssent/shardingsphere
that referenced
this pull request
Apr 9, 2026
…ngsphere/master * remotes/origin/master: Fix COUNT(*) handling for Firebird (apache#37494)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes proposed in this pull request:
COUNT(*), which caused result packet corruption.FirebirdProjectionIdentifierExtractorto supportCOUNT(*)queries without an alias.Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -eUpdate Release Note