fix(sql): reject invalid LIKE/ILIKE escape strings#7810
Open
wjones127 wants to merge 1 commit into
Open
Conversation
Previously the escape character in `LIKE/ILIKE ... ESCAPE '<char>'` was extracted with `char.chars().next()`, which silently accepted invalid input: an empty escape (`ESCAPE ''`) became "no escape", and a multi-character escape (`ESCAPE 'ab'`) was truncated to its first character. Both changed predicate behavior instead of surfacing an error. Extract the shared parsing into `parse_like_escape_char`, which requires exactly one character and returns a descriptive input error otherwise. Fixes lance-format#7804
📝 WalkthroughWalkthroughThe planner adds strict validation for ChangesLIKE escape validation
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
wjones127
marked this pull request as ready for review
July 15, 2026 21:05
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@rust/lance-datafusion/src/planner.rs`:
- Around line 1472-1475: Update the filter cases in the parse_filter test to
include each expression’s expected operator mode, then assert the corresponding
Like case-insensitive flag along with escape_char. Ensure LIKE expects
case-sensitive mode and ILIKE expects case-insensitive mode, while preserving
the existing error handling.
- Around line 1472-1486: Convert the parameterized LIKE/ILIKE test loops in the
planner test to rstest cases: add the appropriate #[rstest] test structure and
represent each filter input with #[case] parameters. Preserve the existing
assertions and rejection behavior for custom, empty, and multi-character escape
values.
- Around line 1487-1492: Update the test around planner.parse_filter to assert
that the returned error matches the concrete invalid-input error variant, while
retaining the existing “Invalid escape character in LIKE expression” message
check. Ensure the assertion rejects other error categories with similar text.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: d2f044be-6198-4d39-b56d-d47148058fc6
📒 Files selected for processing (1)
rust/lance-datafusion/src/planner.rs
Comment on lines
+1472
to
+1475
| for filter in ["s LIKE 'a!%' ESCAPE '!'", "s ILIKE 'a!%' ESCAPE '!'"] { | ||
| match planner.parse_filter(filter).unwrap() { | ||
| Expr::Like(like) => assert_eq!(like.escape_char, Some('!'), "{filter}"), | ||
| other => panic!("expected a LIKE expression for `{filter}`, got {other:?}"), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the operator mode too.
The valid cases only check escape_char, so a regression in the LIKE/ILIKE case-insensitive flag would pass. Include the expected operator mode in each case and assert the corresponding Like flag.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-datafusion/src/planner.rs` around lines 1472 - 1475, Update the
filter cases in the parse_filter test to include each expression’s expected
operator mode, then assert the corresponding Like case-insensitive flag along
with escape_char. Ensure LIKE expects case-sensitive mode and ILIKE expects
case-insensitive mode, while preserving the existing error handling.
Comment on lines
+1472
to
+1486
| for filter in ["s LIKE 'a!%' ESCAPE '!'", "s ILIKE 'a!%' ESCAPE '!'"] { | ||
| match planner.parse_filter(filter).unwrap() { | ||
| Expr::Like(like) => assert_eq!(like.escape_char, Some('!'), "{filter}"), | ||
| other => panic!("expected a LIKE expression for `{filter}`, got {other:?}"), | ||
| } | ||
| } | ||
|
|
||
| // Empty and multi-character escapes are rejected rather than silently | ||
| // dropped or truncated to the first character. | ||
| for filter in [ | ||
| "s LIKE 'x' ESCAPE ''", | ||
| "s LIKE 'x' ESCAPE 'ab'", | ||
| "s ILIKE 'x' ESCAPE ''", | ||
| "s ILIKE 'x' ESCAPE 'ab'", | ||
| ] { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use rstest for these parameterized cases.
These loops differ only by input values and should be expressed with #[rstest] and #[case], as required for Rust parameterized tests.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-datafusion/src/planner.rs` around lines 1472 - 1486, Convert the
parameterized LIKE/ILIKE test loops in the planner test to rstest cases: add the
appropriate #[rstest] test structure and represent each filter input with
#[case] parameters. Preserve the existing assertions and rejection behavior for
custom, empty, and multi-character escape values.
Source: Coding guidelines
Comment on lines
+1487
to
+1492
| let err = planner.parse_filter(filter).unwrap_err(); | ||
| assert!( | ||
| err.to_string() | ||
| .contains("Invalid escape character in LIKE expression"), | ||
| "unexpected error for `{filter}`: {err}" | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the invalid-input error variant as well as its message.
The test currently verifies only a message substring. Match the concrete invalid-input variant too, so a different error category with similar text cannot pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@rust/lance-datafusion/src/planner.rs` around lines 1487 - 1492, Update the
test around planner.parse_filter to assert that the returned error matches the
concrete invalid-input error variant, while retaining the existing “Invalid
escape character in LIKE expression” message check. Ensure the assertion rejects
other error categories with similar text.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously the escape character in
LIKE/ILIKE ... ESCAPE '<char>'was extracted withchar.chars().next(), which silently accepted invalid input:... LIKE 'x' ESCAPE ''→ treated as no escape.... LIKE 'x' ESCAPE 'ab'→ silently truncated to'a'.Both changed predicate behavior instead of surfacing an error, contradicting the "reject invalid values at API boundaries" guideline.
This PR extracts the (previously duplicated) escape parsing into
parse_like_escape_char, which requires exactly one character and returns a descriptive input error for empty or multi-character escapes. Added a test covering the valid single-char case plus empty/multi-char rejection, for bothLIKEandILIKE.Fixes #7804
🤖 Generated with Claude Code
Summary by CodeRabbit
LIKEandILIKEescape characters.