Skip to content

Add multibyte search_path case conversion coverage#1206

Merged
gaoxueyu merged 1 commit intoIvorySQL:masterfrom
Data-Bene:upstream/varlena-identifier-helper
Mar 12, 2026
Merged

Add multibyte search_path case conversion coverage#1206
gaoxueyu merged 1 commit intoIvorySQL:masterfrom
Data-Bene:upstream/varlena-identifier-helper

Conversation

@c2main
Copy link
Copy Markdown
Contributor

@c2main c2main commented Mar 10, 2026

Extend case_conversion.sql to cover multibyte quoted schema names through the search_path code path. The scenario now makes the active identifier_case_switch setting explicit, checks both interchange and normal behavior, and includes a failure case where a quoted schema name stops resolving once the case-conversion mode changes.

Also removed the alternative file output, I suppose it was added by mistake because their diff was very small.

Summary by CodeRabbit

  • Tests
    • Significantly expanded test coverage for SQL identifier case conversion with comprehensive new test sequences.
    • Tests now cover schema lifecycle management, search path configuration, and switching between different case conversion modes.
    • Updated expected test outputs and consolidated test fixtures for better maintainability.
    • Improved test environment isolation with enhanced parameter reset procedures.

Extend case_conversion.sql to cover multibyte quoted schema names
through the search_path code path. The scenario now makes the active
identifier_case_switch setting explicit, checks both interchange and
normal behavior, and includes a failure case where a quoted schema name
stops resolving once the case-conversion mode changes.

Also removed the alternative file output, I suppose it was added by mistake
because their diff was very small.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 10, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f8d86f40-cf8a-4732-9969-11cee9609022

📥 Commits

Reviewing files that changed from the base of the PR and between 0da228b and be7f6e4.

📒 Files selected for processing (3)
  • src/oracle_test/regress/expected/case_conversion.out
  • src/oracle_test/regress/expected/case_conversion_1.out
  • src/oracle_test/regress/sql/case_conversion.sql
💤 Files with no reviewable changes (1)
  • src/oracle_test/regress/expected/case_conversion_1.out

📝 Walkthrough

Walkthrough

Updates regression test fixtures for identifier case conversion feature, removing previous expectations from case_conversion_1.out and expanding case_conversion.out with new schema-based test sequences using Chinese character identifiers. Adds corresponding SQL test setup blocks in case_conversion.sql to exercise identifier_case_switch modes across schema and search_path operations.

Changes

Cohort / File(s) Summary
Case Conversion Test Expectations
src/oracle_test/regress/expected/case_conversion.out, src/oracle_test/regress/expected/case_conversion_1.out
Adds 105 lines of schema test sequences with Chinese character identifiers and search_path operations to case_conversion.out; removes entire 756-line fixture from case_conversion_1.out, consolidating test expectations.
Case Conversion Test SQL
src/oracle_test/regress/sql/case_conversion.sql
Adds 43 lines of schema creation, search_path configuration, and identifier_case_switch mode toggling across interchange, lowercase, and normal modes with corresponding cleanup blocks.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • jiaoshuntian

Poem

🐰 Schemas dance in modes three,
Chinese characters wild and free,
Search paths twist, identifiers gleam—
Case conversion: a tester's dream! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding test coverage for multibyte character handling in search_path case conversion.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/backend/utils/adt/varlena.c`:
- Around line 2757-2766: The code in identifier_case_transform_cstring wrongly
asserts that identifier_case_transform returns the same byte length; change the
check to allow a shorter result and copy only the transformed bytes plus the NUL
terminator instead of assuming equal length. Concretely, replace the
Assert(strlen(new_name) == len) with Assert(strlen(new_name) <= len) (or
equivalent) and use the transformed length (strlen(new_name)) when copying back
into ident (e.g. memcpy/strncpy of strlen(new_name)+1) so truncated names
produced by identifier_case_transform (via
upcase_identifier()/downcase_identifier()) are handled safely before
pfree(new_name).

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 502e88fc-d75d-48e9-89e7-473146567e34

📥 Commits

Reviewing files that changed from the base of the PR and between 0da228b and cb1b0df.

📒 Files selected for processing (4)
  • src/backend/utils/adt/varlena.c
  • src/oracle_test/regress/expected/case_conversion.out
  • src/oracle_test/regress/expected/case_conversion_1.out
  • src/oracle_test/regress/sql/case_conversion.sql
💤 Files with no reviewable changes (1)
  • src/oracle_test/regress/expected/case_conversion_1.out

Comment thread src/backend/utils/adt/varlena.c Outdated
@c2main c2main marked this pull request as draft March 10, 2026 11:26
@c2main c2main force-pushed the upstream/varlena-identifier-helper branch from cb1b0df to be7f6e4 Compare March 10, 2026 14:43
@c2main c2main changed the title Use local helper for identifier case rewrite in varlena Add multibyte search_path case conversion coverage Mar 10, 2026
@c2main c2main marked this pull request as ready for review March 10, 2026 14:45
@c2main
Copy link
Copy Markdown
Contributor Author

c2main commented Mar 10, 2026

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 10, 2026

✅ Actions performed

Full review triggered.

@OreoYang
Copy link
Copy Markdown
Collaborator

@yuanyl630

@gaoxueyu gaoxueyu merged commit f3cf9b3 into IvorySQL:master Mar 12, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants