Skip to content

fix(instr): case-insensitive match and fix wrapped-substr clobber (#3650) - #3729

Open
chiliec wants to merge 1 commit into
dolthub:mainfrom
chiliec:fix-instr-3650
Open

fix(instr): case-insensitive match and fix wrapped-substr clobber (#3650)#3729
chiliec wants to merge 1 commit into
dolthub:mainfrom
chiliec:fix-instr-3650

Conversation

@chiliec

@chiliec chiliec commented Aug 27, 2026

Copy link
Copy Markdown

What

Closes #3650.

INSTR had two independent defects in sql/expression/function/substring.go:

  1. Case-sensitive for nonbinary strings. findSubsequence compared
    runes exactly, so INSTR('xyza','A') returned 0. Per the MySQL 8.4
    manual, INSTR "is multibyte safe, and is case-sensitive only if at
    least one argument is a binary string"
    — it should return 4. This
    mirrors the existing case-insensitive behavior of LOCATE
    (locate.go already lower-cases both sides).

  2. Wrapped substring clobbered the haystack. In Instr.Eval, the
    sql.StringWrapper case of the substring argument assigned the
    unwrapped value to text (the haystack) instead of subtext (the
    needle). A wrapped substring (e.g. Dolt's out-of-band TextStorage
    values) therefore left subtext empty and overwrote text, so
    findSubsequence ran with an empty needle and INSTR returned 1
    unconditionally.

Fix

  • findSubsequence now folds case per rune via a small runeEqualFold
    helper (a == b || unicode.ToLower(a) == unicode.ToLower(b)).
  • The sql.StringWrapper branch of the substring argument now assigns to
    subtext.

+10/-2 in substring.go, plus one new test file.

Tests

Added TestInstrIssue3650 covering both defects (case-insensitive
needle/haystack/mixed + no-match, and wrapped-substring match/no-match/
case-insensitive).

Validation (real results)

Toolchain: Go 1.26.2, -tags gms_pure_go (pure-Go regex engine, no cgo).

  • RED→GREEN proven. With the new test in place and the source fix
    reverted (git checkout on substring.go), all 6 sub-tests fail
    (case-insensitive cases return 0, wrapped-substr cases return 1).
    Restoring the fix turns them green:
    # reverted source, test kept:
    --- FAIL: TestInstrIssue3650/case-insensitive_needle
    --- FAIL: TestInstrIssue3650/wrapped_substr_match
    ... (6 sub-tests fail)
    # with fix:
    ok  github.com/dolthub/go-mysql-server/sql/expression/function
    
  • Existing TestInstr still passes.
  • gofmt -l clean on both files.

The full ./sql/expression/function/ package suite passes except for
TestRegexpReplaceWithFlags/{sensitive_and_multiline_flags,all_flags},
which fail identically on a clean main checkout under the
gms_pure_go tag (a CRLF difference in the pure-Go regex engine) — they
are outside this diff and unrelated to this change.

First-time contributor here — happy to add an enginetest/queries
end-to-end case or adjust anything if you'd prefer.

…b#3650)

INSTR had two defects:

1. It compared runes exactly, so it was case-sensitive for nonbinary
   strings. MySQL's INSTR is multibyte-safe and case-insensitive unless
   one argument is a binary string, e.g. INSTR('xyza','A') should return
   4, not 0.

2. In the StringWrapper branch of the substring argument, the unwrapped
   value was assigned to text (the haystack) instead of subtext (the
   needle). A wrapped substring therefore left subtext empty and
   overwrote the haystack, making INSTR return 1 unconditionally.

findSubsequence now folds case per rune, matching LOCATE's existing
case-insensitive behavior, and the StringWrapper branch assigns to
subtext.
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.

INSTR: case-sensitive for nonbinary strings (should fold), and StringWrapper substring clobbers haystack (returns 1)

2 participants