fix(filter): make k8s CRD backend filter matching case-insensitive#13512
fix(filter): make k8s CRD backend filter matching case-insensitive#13512kaikaila wants to merge 1 commit into
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates filter behavior and tests to support case-insensitive matching for string-based filters (EQ/NEQ/IN/substring), including corresponding SQL generation expectations in select-building tests.
Changes:
- Make
Filter.matchesFilterperform case-insensitive comparisons for EQ, NEQ, IN, and substring operations. - Add test cases validating case-insensitive behavior for SQL select construction and K8s pipeline filtering.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| backend/src/apiserver/filter/filter.go | Switches string comparisons to case-insensitive logic in matchesFilter. |
| backend/src/apiserver/filter/filter_test.go | Adds new test cases asserting case-insensitive behavior for select-building and K8s pipeline filtering. |
| for k := range f.substring { | ||
| fieldVal := fmt.Sprint(getField(k)) | ||
| for _, v := range f.substring[k] { | ||
| if !strings.Contains(fieldVal, fmt.Sprint(v)) { | ||
| if !strings.Contains(strings.ToLower(fieldVal), strings.ToLower(fmt.Sprint(v))) { | ||
| return false, nil | ||
| } | ||
| } |
| fieldVal := fmt.Sprint(getField(k)) | ||
| for _, v := range f.substring[k] { | ||
| if !strings.Contains(fieldVal, fmt.Sprint(v)) { | ||
| if !strings.Contains(strings.ToLower(fieldVal), strings.ToLower(fmt.Sprint(v))) { |
There was a problem hiding this comment.
IS_SUBSTRING uses strings.ToLower while EQ/NEQ/IN use strings.EqualFold, which means Unicode case folding edge cases (e.g. ß vs ss) are handled inconsistently between operators. This is a known limitation — MLflow takes the same approach (re.IGNORECASE for substring, no Unicode fold normalization) and accepts it without further abstraction. Fixing this properly would require a Unicode-aware fold for substring matching, which is out of scope for this PR.
81a83fa to
134a195
Compare
|
/retest |
| fieldVal := fmt.Sprint(getField(k)) | ||
| for _, v := range f.eq[k] { | ||
| if fieldVal != fmt.Sprint(v) { | ||
| if !strings.EqualFold(fieldVal, fmt.Sprint(v)) { |
There was a problem hiding this comment.
Hey @kaikaila , I'm not seeing this case insensitive behavior in the code. Is it possibly a MySQL behavioral quirk you're experiencing?
func (f *Filter) AddToSelect(sb squirrel.SelectBuilder) squirrel.SelectBuilder {
for k := range f.eq {
for _, v := range f.eq[k] {
m := map[string]interface{}{k: v}
sb = sb.Where(squirrel.Eq(m)) // plain "="; no LOWER(...)
}
}
for k := range f.neq {
for _, v := range f.neq[k] {
m := map[string]interface{}{k: v}
sb = sb.Where(squirrel.NotEq(m)) // plain "<>"; no LOWER(...)
}
}
// ...
for k := range f.substring {
for _, v := range f.substring[k] {
like := make(squirrel.Like)
like[k] = fmt.Sprintf("%%%s%%", v)
sb = sb.Where(like) // plain LIKE; no ILIKE / LOWER(...)
}
}
return sb
}There was a problem hiding this comment.
Hey @mprahl, thanks for the feedback!
To clarify my intent here: I believe KFP's filter API should behave consistently regardless of which pipeline store backend is used. Right now, the K8s pipeline store case-sensitive collation causes filter behavior to silently differ from MySQL's— and I don't think that's intentional.
So I'd love to get your or the community's input on what the right policy should be:
A. Case-insensitive everywhere (my current fix. align to MySQL behavior, fix K8s pipeline store to match, and postgres in #12379)
B. Case-sensitive everywhere (align to K8s pipeline store / postgres behavior, breaking change for MySQL users)
C. Leave behavior backend-dependent (K8s pipeline store differs from MySQL — users get different filter semantics depending on their deployment)
Happy to implement whichever direction is decided.
For reference, MLflow faced the same problem and they force case-sensitive behavior across all backends by using BINARY in MySQL queries. See here.
134a195 to
707d998
Compare
Signed-off-by: kaikaila <lyk2772@126.com>
707d998 to
5bad05a
Compare
Problem
The
matchesFilter()function used by the k8s CRD pipeline store backend performs case-sensitive string comparisons forEQ,NEQ,IN, andIS_SUBSTRINGpredicates. This is inconsistent with the SQL database backends (MySQL/PostgreSQL), which already applyLOWER()wrapping and are therefore case-insensitive.As a result, a filter like
name = "My-Pipeline"would match in the database backend but not in the k8s CRD backend.Fix
Replace direct string equality and
strings.Containscalls inmatchesFilter()with case-insensitive equivalents:EQ/NEQ:strings.EqualFoldIN:strings.EqualFoldIS_SUBSTRING:strings.ToLoweron both sides beforestrings.ContainsTests
Added case-insensitive test cases for all four affected operators in both
FilterK8sPipelines(k8s CRD path) andAddToSelect/AddToSelectV1(SQL path).Notes
This fix addresses application-layer behavioral inconsistency — it is distinct from the cross-backend collation ordering differences (e.g.
-vs_sort order between MySQL and PostgreSQL), which are a known database-level limitation and are documented separately.This commit was originally authored on
feature/postgres-integrationand is being submitted as a standalone fix targeting master.