Skip to content

Fix COUNT(*) handling for Firebird#37494

Merged
terrymanu merged 3 commits into
apache:masterfrom
makssent:dev
Dec 25, 2025
Merged

Fix COUNT(*) handling for Firebird#37494
terrymanu merged 3 commits into
apache:masterfrom
makssent:dev

Conversation

@makssent
Copy link
Copy Markdown
Contributor

Changes proposed in this pull request:

  • Fix COUNT(*) handling for Firebird
    • Fixed an issue where an incorrect data type was assigned to COUNT(*), which caused result packet corruption.
    • Added a Firebird-specific FirebirdProjectionIdentifierExtractor to support COUNT(*) queries without an alias.

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:
    ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e
  • 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

@makssent makssent marked this pull request as ready for review December 24, 2025 12:05
Copy link
Copy Markdown
Member

@terrymanu terrymanu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • 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.

@makssent
Copy link
Copy Markdown
Contributor Author

makssent commented Dec 24, 2025

  • 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.

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!

@makssent makssent marked this pull request as draft December 24, 2025 13:52
@makssent makssent marked this pull request as ready for review December 24, 2025 15:16
@terrymanu terrymanu merged commit 5f970d1 into apache:master Dec 25, 2025
145 checks passed
@terrymanu terrymanu added this to the 5.5.3 milestone 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants