Skip to content

chore(sql): fix false-negative results in Utf8s#isAscii()#6556

Merged
bluestreak01 merged 2 commits intomasterfrom
puzpuzpuz_fix_isascii
Dec 20, 2025
Merged

chore(sql): fix false-negative results in Utf8s#isAscii()#6556
bluestreak01 merged 2 commits intomasterfrom
puzpuzpuz_fix_isascii

Conversation

@puzpuzpuz
Copy link
Copy Markdown
Contributor

@puzpuzpuz puzpuzpuz commented Dec 18, 2025 •
edited
Loading

The only place where this utility method is used is RegexpReplaceVarcharFunctionFactory:

// somewhere in the SingleGroupAsciiFunc class:

                // If the string is non-ASCII, we need to recalculate
                // the ASCII flag for the matched substring.
                boolean ascii = view.isAscii() || Utf8s.isAscii(ptr, size);

@puzpuzpuz puzpuzpuz self-assigned this Dec 18, 2025
@puzpuzpuz puzpuzpuz added Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution labels Dec 18, 2025
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 18, 2025 •
edited
Loading

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Walkthrough

Fixed a logic bug in Utf8s.isAscii(long ptr, int size) where the per-8-byte check was incorrectly handling ASCII words. The conditional now returns false only when non-ASCII data is detected, enabling correct short-circuit behavior. Extended test coverage validates the fix.

Changes

Cohort / File(s) Change Summary
Logic Fix
core/src/main/java/io/questdb/std/str/Utf8s.java		 Corrected conditional logic in isAscii(long ptr, int size) to return false on non-ASCII 8-byte words and continue on ASCII words; tail-byte validation unchanged
Test Extension
core/src/test/java/io/questdb/test/std/str/Utf8sTest.java		 Added test steps in testIsAscii to validate ASCII detection on longer strings using both sink and pointer/size method variants

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

  • Logic fix is localized to a single conditional in one method
  • Test extension follows existing patterns with straightforward assertions
  • No public API or signature changes

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: fixing a logic bug in Utf8s#isAscii() that was producing false-negative results. It is concise and directly related to the changeset.
Description check ✅ Passed The description is related to the changeset, providing context about where Utf8s#isAscii() is used (RegexpReplaceVarcharFunctionFactory) and demonstrating the practical impact of the bug fix.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@puzpuzpuz
Copy link
Copy Markdown
Contributor Author

puzpuzpuz commented Dec 18, 2025

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Dec 18, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@glasstiger
Copy link
Copy Markdown
Contributor

glasstiger commented Dec 20, 2025

[PR Coverage check]

😍 pass : 1 / 1 (100.00%)

file detail

path covered line new line coverage
🔵 io/questdb/std/str/Utf8s.java 1 1 100.00%

@bluestreak01 bluestreak01 merged commit e407fe3 into master Dec 20, 2025
43 checks passed
@bluestreak01 bluestreak01 deleted the puzpuzpuz_fix_isascii branch December 20, 2025 21:38
@coderabbitai coderabbitai bot mentioned this pull request Jan 20, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bluestreak01 bluestreak01 bluestreak01 approved these changes

Assignees

@puzpuzpuz puzpuzpuz

Labels

Bug Incorrect or unexpected behavior SQL Issues or changes relating to SQL execution

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@puzpuzpuz @glasstiger @bluestreak01