[BUG] PPL LIKE function: no way to escape the escape character (\)

[AjimelecGonzalez]

Description

When the DEFAULT_ESCAPE (\) case is hit, the code sets escaped = true without checking whether escaped is already true. This means a sequence of two backslashes (\\, which represents a literal backslash in LIKE syntax) sets escaped = true twice, the second backslash does not consume the escape state from the first. As a result, escaped remains true after both backslashes, causing the next character (e.g., %) to be incorrectly treated as escaped (literal) instead of as a wildcard.

The fix adds a check: if escaped is already true when we encounter another backslash, we know this backslash is being escaped by the previous one. We treat it as a literal backslash (removing the escape prefix from the output) and reset escaped to false.

Related Issues

Resolves #5627

Check List

• New functionality includes testing.
• New functionality has been documented.
• New functionality has javadoc added.
• New functionality has a user manual doc added.
• New PPL command checklist all confirmed.
• API changes companion pull request created.
• Commits are signed per the DCO using --signoff or -s.
• Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit 531e974)

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Incomplete Fix The fix handles escaped backslashes but does not reset the escaped flag for other characters when escaped is true. After processing an escaped character (like % or _), the code should set escaped = false to prevent the flag from carrying over. Without this, a sequence like \%a would incorrectly treat a as escaped if the % case doesn't reset the flag. Review all case branches to ensure escaped is reset after consuming an escape sequence. switch (currentChar) { case DEFAULT_ESCAPE: if (escaped) { convertedString.deleteCharAt(convertedString.length() - 1); convertedString.append(currentChar); escaped = false; } else { escaped = true; convertedString.append(currentChar); } break;

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Prevent IndexOutOfBoundsException on empty string The logic for handling escaped backslashes may fail when convertedString is empty. If the first character is a backslash and escaped is true, calling deleteCharAt on an empty string will throw an IndexOutOfBoundsException. Add a bounds check before deleting. opensearch/src/main/java/org/opensearch/sql/opensearch/storage/script/StringUtils.java [46-54] case DEFAULT_ESCAPE: if(escaped) { - convertedString.deleteCharAt(convertedString.length() - 1); + if (convertedString.length() > 0) { + convertedString.deleteCharAt(convertedString.length() - 1); + } convertedString.append(currentChar); escaped = false; } else { escaped = true; convertedString.append(currentChar); } break; Suggestion importance[1-10]: 9 __ Why: This is a critical bug fix. The code attempts to delete a character from convertedString without checking if it's empty, which would cause an IndexOutOfBoundsException when the first character is a backslash and escaped is true. The suggested bounds check is essential for preventing runtime crashes.	High
	Handle null argument safely The check for existing percent field occurs after showPerc is retrieved but doesn't verify if the argument exists. If argumentMap.get() returns null, calling getValue() will throw a NullPointerException. Ensure the argument exists before accessing its value. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [3234-3240] -Boolean showPerc = (Boolean) argumentMap.get(RareTopN.Option.showPerc.name()).getValue(); +Argument showPercArg = argumentMap.get(RareTopN.Option.showPerc.name()); +Boolean showPerc = showPercArg != null ? (Boolean) showPercArg.getValue() : false; if (showPerc) { if (context.relBuilder.peek().getRowType().getFieldNames().contains("percent")) { throw new IllegalArgumentException( "Field `percent` already exists in the output. Consider renaming the conflicting" + " field."); } Suggestion importance[1-10]: 2 __ Why: While the suggestion is technically correct about null safety, the showPerc argument is added by ArgumentFactory.getArgumentList() with a default value of Literal.FALSE (line 314 in the PR), ensuring it always exists in the argumentMap. The null check is unnecessary given the current implementation, making this a low-priority suggestion.	Low

[github-actions]

Persistent review updated to latest commit 15ab747

[github-actions]

Persistent review updated to latest commit 531e974