From 82e7d7d3b1fc482d6ce489aad0a57bb15c63f744 Mon Sep 17 00:00:00 2001 From: Vladimir Babin Date: Thu, 27 Aug 2026 11:31:21 +0300 Subject: [PATCH] fix(instr): case-insensitive match and wrapped-substr clobber (#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. --- .../function/instr_issue3650_test.go | 66 +++++++++++++++++++ sql/expression/function/substring.go | 12 +++- 2 files changed, 76 insertions(+), 2 deletions(-) create mode 100644 sql/expression/function/instr_issue3650_test.go diff --git a/sql/expression/function/instr_issue3650_test.go b/sql/expression/function/instr_issue3650_test.go new file mode 100644 index 0000000000..7e0c9ab1b4 --- /dev/null +++ b/sql/expression/function/instr_issue3650_test.go @@ -0,0 +1,66 @@ +// Copyright 2020-2021 Dolthub, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package function + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/dolthub/go-mysql-server/sql" + "github.com/dolthub/go-mysql-server/sql/expression" + "github.com/dolthub/go-mysql-server/sql/types" + "github.com/dolthub/go-mysql-server/test" +) + +// TestInstrIssue3650 covers the two defects reported in +// dolthub/go-mysql-server#3650: +// 1. INSTR was case-sensitive for nonbinary strings; MySQL's INSTR is +// case-insensitive unless one argument is a binary string. +// 2. A wrapped substring argument clobbered the haystack, so INSTR +// returned 1 unconditionally for any StringWrapper needle. +func TestInstrIssue3650(t *testing.T) { + f := NewInstr( + sql.NewEmptyContext(), + expression.NewGetField(0, types.LongText, "str", true), + expression.NewGetField(1, types.LongText, "substr", false), + ) + + testCases := []struct { + name string + row sql.Row + expected int + }{ + // Defect 1: case-insensitive for nonbinary strings. + {"case-insensitive needle", sql.NewRow("xyza", "A"), 4}, + {"case-insensitive haystack", sql.NewRow("XYZA", "a"), 4}, + {"case-insensitive mixed", sql.NewRow("Hello World", "world"), 7}, + {"case-insensitive no match", sql.NewRow("abc", "Z"), 0}, + // Defect 2: wrapped substring must NOT overwrite the haystack. + {"wrapped substr match", sql.NewRow("foobar", test.NewMockStringWrapper("bar")), 4}, + {"wrapped substr no match", sql.NewRow("foobar", test.NewMockStringWrapper("xyz")), 0}, + {"wrapped substr case-insensitive", sql.NewRow("foobar", test.NewMockStringWrapper("BAR")), 4}, + } + + for _, tt := range testCases { + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + ctx := sql.NewEmptyContext() + v, err := f.Eval(ctx, tt.row) + require.NoError(err) + require.Equal(int64(tt.expected), v) + }) + } +} diff --git a/sql/expression/function/substring.go b/sql/expression/function/substring.go index 3b933133da..809270e7ac 100644 --- a/sql/expression/function/substring.go +++ b/sql/expression/function/substring.go @@ -18,6 +18,7 @@ import ( "fmt" "reflect" "strings" + "unicode" "github.com/dolthub/go-mysql-server/sql" "github.com/dolthub/go-mysql-server/sql/types" @@ -654,7 +655,7 @@ func (i Instr) Eval(ctx *sql.Context, row sql.Row) (interface{}, error) { if err != nil { return nil, err } - text = []rune(s) + subtext = []rune(s) case []byte: subtext = []rune(string(substr)) case sql.BytesWrapper: @@ -676,7 +677,7 @@ func findSubsequence(text []rune, subtext []rune) int64 { for i := 0; i <= len(text)-len(subtext); i++ { var j int for j = 0; j < len(subtext); j++ { - if text[i+j] != subtext[j] { + if !runeEqualFold(text[i+j], subtext[j]) { break } } @@ -687,6 +688,13 @@ func findSubsequence(text []rune, subtext []rune) int64 { return -1 } +// runeEqualFold reports whether two runes are equal under simple case folding. +// MySQL's INSTR is case-insensitive unless one argument is a binary string, so +// the rune comparison folds case to match that behavior. +func runeEqualFold(a, b rune) bool { + return a == b || unicode.ToLower(a) == unicode.ToLower(b) +} + // IsNullable implements the Expression interface. func (i Instr) IsNullable(ctx *sql.Context) bool { return i.str.IsNullable(ctx) || i.substr.IsNullable(ctx)