Skip to content

refactor(c/driver_manager): replace std::regex in profile interpolation - #4780

Open
kevinushey wants to merge 3 commits into
apache:mainfrom
kevinushey:bugfix/remove-std-regex
Open

refactor(c/driver_manager): replace std::regex in profile interpolation#4780
kevinushey wants to merge 3 commits into
apache:mainfrom
kevinushey:bugfix/remove-std-regex

Conversation

@kevinushey

@kevinushey kevinushey commented Sep 12, 2026

Copy link
Copy Markdown

Profile interpolation currently constructs a std::regex to expand {{ env_var(NAME) }} expressions. Replace it with a linear scanner that preserves whitespace trimming, overlapping delimiters, literal malformed braces, and existing error messages. This removes the regex construction path implicated in #4638.

Keep the Go driver manager copies synchronized and remove the remaining unused <regex> includes. Add regression coverage for delimiter edge cases, whitespace, adjacent substitutions, embedded NULs, long brace sequences, errors, and nonrecursive expansion.

The interpolation helper follows the existing internal test-helper export convention (InternalAdbc prefix and ADBC_EXPORT) so shared-library tests link on Linux and Windows.

The R check workflow explicitly skips Pandoc installation, which these packages do not need. This avoids the action's automatic dependency probe, which ignores the local driver manager override and fails to find the archived package on CRAN.

Validation:

  • Driver manager suite on macOS: 179 passed, 28 skipped.
  • Executed the pinned Pandoc detection step with the workflow setting for all four R packages; all completed successfully. YAML and action pin checks passed.
  • Linux: built the shared driver manager with c/symbols.map, linked and ran interpolation success/error checks, and verified the helper is exported.
  • A temporary differential harness compared status, output, and errors against the original implementation for 187,381 exhaustive and randomized inputs with AddressSanitizer and UndefinedBehaviorSanitizer; all matched.
  • Pre-commit checks passed for formatting, cpplint, licenses, spelling, whitespace, and C/Go source synchronization. The always-run C# formatting hook could not run because dotnet is unavailable; no C# files changed.

The CRAN GCC 16 sanitizer environment was not reproduced locally.

Closes #4638

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approval recommended

No unresolved blocking issues were identified.

Pull request overview

Replaces regex-based profile interpolation with a linear scanner while preserving existing behavior and adding edge-case coverage.

Changes:

  • Adds regression tests for delimiters, whitespace, malformed input, NULs, errors, and nonrecursive expansion.
  • Synchronizes C and Go implementations.
  • Removes unused regex includes.
File summaries
File Description
go/adbc/drivermgr/adbc_driver_manager.cc Removes unused regex include
go/adbc/drivermgr/adbc_driver_manager_profiles.cc Synchronized scanner implementation
c/driver_manager/adbc_driver_manager.cc Removes unused regex include
c/driver_manager/adbc_driver_manager_test.cc Adds regression coverage
c/driver_manager/adbc_driver_manager_profiles.cc Implements scanner-based interpolation
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@kevinushey kevinushey changed the title refactor(c/driver-manager): replace std::regex in profile interpolation refactor(c/driver_manager): replace std::regex in profile interpolation Sep 12, 2026
@paleolimbot

Copy link
Copy Markdown
Member

Thanks Kevin!

+1 on the approach, although I'll let David and Matt weigh in on the implementation details (I haven't followed the syntax for env var escapes).

The error coming off of the check jobs isn't all that helpful...I can investigate this evening. That workflow hasn't been updated for some time.

I haven't heard back from CRAN about the fix that I had submitted...I am guessing it didn't work which is why they archived it, although I do wish they had let me know first 😮‍💨 . I'll pick this and push to a tweaked CRAN release this evening in case that's somehow faster.

@zeroshade zeroshade left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks okay to me, but we should fix the failing CI first

@kevinushey

Copy link
Copy Markdown
Author

I can try to run down the failing CI, but it might take a few iterations (especially since I'll need a maintainer to help run the workflows as I make changes)

@paleolimbot paleolimbot left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you!

paleolimbot added a commit to paleolimbot/arrow-adbc that referenced this pull request Sep 13, 2026
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.

Address sanitizer issue in adbcdrivermanager R package

4 participants