[anohedr]

Description

Improves the error message returned when an index has an empty mapping. This change makes the error clearer and more actionable for users while preserving the existing behavior. Unit tests have been updated to verify the new error message.

Related Issues

Resolves #[4872]

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 fdcf469)

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 Possible Issue The pattern detection logic treats a single-element array containing a comma-separated string (e.g., "index1,index2") as a non-pattern, but OpenSearch interprets commas as multiple indices. This mismatch means users querying "index1,index2" when neither exists will see "No index found for 'index1,index2'" instead of the "compatible mapping" hint, even though OpenSearch treats this as a multi-index pattern that could have mapping conflicts. || Arrays.stream(exprs) .filter(e -> e != null) .anyMatch( e -> e.contains("*") || e.contains("?") || e.startsWith("<") || e.startsWith("-") || e.equals("_all"));

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to fdcf469

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Fix false positive pattern detection The pattern detection logic treats a leading dash (-) as an exclusion pattern, but this incorrectly flags negative numbers or date-based indices like logs-2024 as patterns. Exclusions in OpenSearch require a preceding inclusion pattern (e.g., logs,-logs-old). A standalone name starting with - should not be treated as a pattern.* opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchClient.java [144-154] boolean isPattern = exprs.length > 1 || Arrays.stream(exprs) .filter(e -> e != null) .anyMatch( e -> e.contains("*") || e.contains("?") || e.startsWith("<") - || e.startsWith("-") || e.equals("_all")); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that e.startsWith("-") can incorrectly flag non-pattern indices like logs-2024 as patterns. However, the test at line 306 (empty_mapping_exception_exclusion_produces_compatible_mapping_message) explicitly expects -logs-old to be treated as a pattern, suggesting the current behavior may be intentional for exclusion syntax.	Medium

---

Previous suggestions

Suggestions up to commit 1bb7e9e

Category	Suggestion	Impact
General	Remove incorrect dash pattern check The pattern detection logic treats a leading dash (-) as a wildcard pattern indicator, but in OpenSearch index expressions, a leading dash denotes exclusion (e.g., index,-excluded). This can cause false positives when checking for patterns. Consider removing the e.startsWith("-") check or refining the logic to handle exclusion syntax correctly.* opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchClient.java [144-154] boolean isPattern = exprs.length > 1 || Arrays.stream(exprs) .filter(e -> e != null) .anyMatch( e -> e.contains("*") || e.contains("?") || e.startsWith("<") - || e.startsWith("-") || e.equals("_all")); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that e.startsWith("-") may cause false positives since a leading dash in OpenSearch denotes exclusion syntax, not a wildcard pattern. Removing this check would improve the accuracy of pattern detection, though the impact is moderate since exclusion patterns are less common.	Medium

Suggestions up to commit 879940e

Category	Suggestion	Impact
General	Remove incorrect hyphen pattern check The pattern detection logic treats a leading hyphen (-) as a wildcard pattern indicator, but in OpenSearch index expressions, a leading hyphen denotes exclusion (e.g., index,-excluded). This can cause false positives when checking for patterns. Consider removing the e.startsWith("-") condition or refining the logic to distinguish between exclusion syntax and actual pattern indicators.* opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchClient.java [144-154] boolean isPattern = exprs.length > 1 || Arrays.stream(exprs) .filter(e -> e != null) .anyMatch( e -> e.contains("*") || e.contains("?") || e.startsWith("<") - || e.startsWith("-") || e.equals("_all")); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that e.startsWith("-") may cause false positives in pattern detection. In OpenSearch, a leading hyphen indicates exclusion syntax rather than a wildcard pattern. Removing this check would improve the accuracy of pattern detection, though the impact is moderate since exclusion syntax is less commonly used alone.	Medium

Suggestions up to commit 21ff0fc

Category	Suggestion	Impact
Possible issue	Avoid false pattern detection for hyphenated names The pattern detection logic treats a leading hyphen (-) as a wildcard pattern indicator, but this conflicts with date-based index names (e.g., logs-2024-01-01) which are common and not patterns. Consider checking for exclusion syntax more precisely (e.g., -index without other characters) or removing this check to avoid false positives. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchClient.java [144-154] boolean isPattern = exprs.length > 1 || Arrays.stream(exprs) .filter(e -> e != null) .anyMatch( e -> e.contains("*") || e.contains("?") || e.startsWith("<") - || e.startsWith("-") || e.equals("_all")); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that e.startsWith("-") can cause false positives for date-based index names like logs-2024-01-01. However, the check is intended to detect exclusion patterns (e.g., -excluded_index), which are valid OpenSearch syntax. The suggestion to remove this check entirely may miss legitimate exclusion patterns, but the concern about false positives is valid and warrants careful consideration.	Medium

Suggestions up to commit 5e50c14

Category	Suggestion	Impact
General	Fix pattern detection for exclusions The pattern detection logic treats any expression starting with - as a pattern, but - is used for exclusions in multi-index patterns (e.g., index,-excluded). A single expression starting with - (like -myindex) is invalid and should not be classified as a pattern requiring the "compatible mapping" hint.* opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchClient.java [144-154] boolean isPattern = exprs.length > 1 || Arrays.stream(exprs) .filter(e -> e != null) .anyMatch( e -> e.contains("*") || e.contains("?") || e.startsWith("<") - || e.startsWith("-") || e.equals("_all")); Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that a single expression starting with - (like -myindex) is invalid and shouldn't trigger the "compatible mapping" hint. However, the logic exprs.length > 1 already handles multi-index patterns where exclusions are valid, so removing the e.startsWith("-") check is appropriate for single-expression cases.	Medium

Suggestions up to commit 8f53d9a

Category	Suggestion	Impact
General	Remove redundant ErrorReport catch block Catching and re-throwing ErrorReport is unnecessary since ErrorReport extends RuntimeException. The existing catch block for generic Exception will not catch ErrorReport anyway due to the order. Remove this redundant catch block to simplify the exception handling flow. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchNodeClient.java [117-119] -} catch (ErrorReport e) { - throw e; } catch (Exception e) { Suggestion importance[1-10]: 4 __ Why: The catch block for ErrorReport appears redundant since it only re-throws the exception. However, it serves to preserve the specific ErrorReport type before the generic Exception handler wraps it in IllegalStateException. Removing it would change exception handling behavior, though the impact is minor since ErrorReport extends RuntimeException.	Low
	Fix pattern detection for multi-index queries The pattern detection logic treats a comma within a single string as a pattern indicator, but commas are also used to separate multiple index names in a single argument. This can cause false positives where legitimate multi-index queries without wildcards are incorrectly flagged as patterns. Consider checking if the array contains multiple elements OR if any single element contains pattern syntax. opensearch/src/main/java/org/opensearch/sql/opensearch/client/OpenSearchClient.java [134-146] String[] exprs = indexExpression != null ? indexExpression : new String[0]; String joined = String.join(",", exprs); boolean isPattern = - Arrays.stream(exprs) - .filter(e -> e != null) - .anyMatch( - e -> - e.contains("*") - || e.contains("?") - || e.contains(",") - || e.startsWith("<") - || e.startsWith("-") - || e.equals("_all")); + exprs.length > 1 + || Arrays.stream(exprs) + .filter(e -> e != null) + .anyMatch( + e -> + e.contains("*") + || e.contains("?") + || e.contains(",") + || e.startsWith("<") + || e.startsWith("-") + || e.equals("_all")); Suggestion importance[1-10]: 3 __ Why: The suggestion identifies a potential issue where exprs.length > 1 could help distinguish multi-index queries, but the current logic already handles commas within strings as pattern indicators (which is correct per OpenSearch syntax). The test at line 310-314 validates this behavior. The suggestion may introduce unintended changes to the error message logic without clear benefit.	Low

[github-actions]

Persistent review updated to latest commit 8f53d9a

[github-actions]

Persistent review updated to latest commit 5e50c14

[github-actions]

Persistent review updated to latest commit 21ff0fc

[github-actions]

Persistent review updated to latest commit 5618a2e

[github-actions]

Persistent review updated to latest commit 879940e

[github-actions]

Persistent review updated to latest commit 1bb7e9e

[codecov]

Codecov Report

❌ Patch coverage is 87.09677% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.20%. Comparing base (9249643) to head (1bb7e9e).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
...search/sql/opensearch/client/OpenSearchClient.java	85.18%	0 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #5609   +/-   ##
=========================================
  Coverage     75.19%   75.20%           
- Complexity    10362    10372   +10     
=========================================
  Files           962      963    +1     
  Lines         41098    41127   +29     
  Branches       4683     4691    +8     
=========================================
+ Hits          30905    30930   +25     
  Misses         8896     8896           
- Partials       1297     1301    +4     

Flag	Coverage Δ
sql-engine	75.20% <87.09%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Persistent review updated to latest commit fdcf469