[Bug] fix case sensitivity for wildcard queries

[nknize]

Description

Fixes the wildcard query to not normalize the pattern when case_insensitive is set by the user. This is achieved by creating a new normalizedWildcardQuery method so that query_string queries (which do not support case sensitivity) can still normalize the pattern when the default analyzer is used; maintaining existing behavior.

Issues Resolved

closes #5461

Check List

• New functionality includes testing.
  • All tests pass
• New functionality has been documented.
  • New functionality has javadoc added
• Commits are signed per the DCO using --signoff
• Commit changes are listed out in CHANGELOG.md file (See: Changelog)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/7655/
• CommitID: 539ba18
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/7656/
• CommitID: 0e67e6f
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/7661/
• CommitID: 1081116
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[reta]

@nknize I think we still do have a conflict here: it makes sense to apply the caseInsensitive hint when user does not specify the analyzer to be used (in this case getTextSearchInfo().getSearchAnalyzer() is the default one), but I think we should not do that if the analyzer or/and analyze_wildcard are provided

[1] https://www.elastic.co/guide/en/elasticsearch/reference/7.10/query-dsl-query-string-query.html

[nknize]

that's why QueryStringQueryParser calls normalizedWildcardQuery method. It ignores the caseInsensitive parameter for query string queries.

[reta]

I think that was caused the confusion to me: we already have 2 variants of wildcardQuery method + now a new one called normalizedWildcardQuery, could we drop normalizedWildcardQuery since this is just delegate to wildcardQuery ?

[nknize]

We sure can.. it's just syntactic sugar. I just thought it was a more descriptive call in the QueryStringQueryParser than calling wildcardQuery(value, method, false, true, context) , but I can throw javadocs there to describe the logic.

[nknize]

ah wait.. nvm..I did it this way because the delegation for normalizedWildcardQuery is specific to StringFieldType only... It's not the same for keyword and constant fields.

[reta]

I think this is non issue: since you have this new method

public Query wildcardQuery(
        String value,
        MultiTermQuery.RewriteMethod method,
        boolean caseInsensitive,
        boolean normalizeIfAnalyzed,
        QueryShardContext context
    )

only StringFieldType could implement it, others won't. Another option I was thinking, may be we could embed this into TextSearchInfo / MappedFieldType as a property, since as you mentioned it is specific to StringFieldType only?

[nknize]

What I mean is QueryStringQueryParser calls the generic MappedFieldType.wildcardQuery method. I can eliminate the normalized version of this method call by bumping the new method up the class hierarchy but I'd prefer not do that since normalizeIfAnalyzed is only relevant to the StringFIeldType. (and I don't like the idea of having to keep track of what the caseInsensitive and normalizeIfAnalyzed boolean logic means if we add another field type that doesn't care).

I think this API is cleaner? But I should add javadocs to describe that normalizeWildcardQuery normalizes to lower case whereas wildcardQuery optionally lowers case.

[reta]

Javadocs would help for sure, I don't think my suggestion (to have overloaded wildcardQuery) is any better than new normalizedWildcardQuery - removes confusion in one place but hurts another one. What do you think about enriching TextSearchInfo for StringFieldType? (than we don't need this normalizeIfAnalyzed argument at all) I believe this could be exactly the right place to do so:

/**
 * Encapsulates information about how to perform text searches over a field
 *
 * @opensearch.internal
 */
public class TextSearchInfo {
  ...
}

[nknize]

What do you think about enriching TextSearchInfo for StringFieldType?

The problem is QueryStringQueryParser needs to change the normalization behavior at search runtime and TextSearchInfo is defined at index creation time for encapsulating Lucene fieldType parameters (e.g., norms, offsets, positions). This could be hacked around by adding a setter to dynamically change TextSearchInfo based on the query but that changes the purpose of TextSearchInfo and I think adds confusion on when to do this and for what type of queries. It might be worth exploring but I think in a follow up enhancement issue since the surface area impacted is larger than the scope of this bug fix? This may also highlight that TextSearchInfo isn't the best classname.

[reta]

I see thanks @nknize

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/7681/
• CommitID: 378310b

[codecov-commenter]

Codecov Report

Merging #5462 (378310b) into main (d266a73) will decrease coverage by 0.16%.
The diff coverage is 58.33%.

@@             Coverage Diff              @@
##               main    #5462      +/-   ##
============================================
- Coverage     71.10%   70.93%   -0.17%     
+ Complexity    58238    58154      -84     
============================================
  Files          4711     4711              
  Lines        277573   277579       +6     
  Branches      40180    40182       +2     
============================================
- Hits         197358   196912     -446     
- Misses        64078    64530     +452     
  Partials      16137    16137              

Impacted Files	Coverage Δ
...rg/opensearch/index/mapper/KeywordFieldMapper.java	86.04% <0.00%> (-0.51%)	⬇️
...a/org/opensearch/index/mapper/MappedFieldType.java	74.74% <25.00%> (-1.55%)	⬇️
...a/org/opensearch/index/mapper/StringFieldType.java	85.07% <66.66%> (-5.70%)	⬇️
...h/cluster/service/ClusterManagerTaskThrottler.java	87.83% <100.00%> (+0.16%)	⬆️
...pensearch/index/search/QueryStringQueryParser.java	70.47% <100.00%> (ø)
...adonly/AddIndexBlockClusterStateUpdateRequest.java	0.00% <0.00%> (-75.00%)	⬇️
...readonly/TransportVerifyShardIndexBlockAction.java	9.75% <0.00%> (-73.18%)	⬇️
...a/org/opensearch/client/cluster/ProxyModeInfo.java	0.00% <0.00%> (-55.00%)	⬇️
.../opensearch/client/indices/CloseIndexResponse.java	42.50% <0.00%> (-48.75%)	⬇️
...adcast/BroadcastShardOperationFailedException.java	55.55% <0.00%> (-44.45%)	⬇️
... and 467 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: FAILURE ❌
• URL: https://build.ci.opensearch.org/job/gradle-check/7786/
• CommitID: 8cea795
  Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green.
  Is the failure a flaky test unrelated to your change?

[github-actions]

Gradle Check (Jenkins) Run Completed with:

• RESULT: SUCCESS ✅
• URL: https://build.ci.opensearch.org/job/gradle-check/7787/
• CommitID: 7e73510