fix(queryEnhancements): data.search() should not ignore the strategy passed as parameter

[ruanyl]

This is quick fix for the issue that the search strategy passed to data.search() API been completely ignored when query enhancement is enabled and the localStorage has a language preserved.

For example, selecting a language from discover language selector will make data.search() called with a specific strategy to use the language config stored in localStorage which is unexpected.

With this fix, language config is only used when search strategy is not passed explicitly.

Sharing my two cents:

1. I think it's better the language is passed as a parameter to data.search() instead of reading directly from query string manager(which has default value from local storage that different feature could share the same value). I think data.search() needs to be UI/feature-agnostic
2. I'm fine with having different search intercepters for different languages, but it seems the current intercepters(ppl & sql) does way too much than than an "intercepter" should. IMHO, a search intercepter will just intercepts the search params(input) and search response(output), and does things like error handling, validation, etc. But now it seems we're building the search query in the intercepter by reading from some global state: time fitter, aggregation, etc. Shouldn't this be done upfront and passed to data.search() as search params?

I may not have a full picture of the entire idea of query enhancement, and I'm open to any other ideas :)

Description

Issues Resolved

Screenshot

Testing the changes

Changelog

• fix: data.search() should not ignore the strategy passed as parameter

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[github-actions]

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

[github-actions]

❌ Empty Changelog Section

The Changelog section in your PR description is empty. Please add a valid changelog entry or entries. If you did add a changelog entry, check to make sure that it was not accidentally included inside the comment block in the Changelog section.

[codecov]

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 60.92%. Comparing base (8691010) to head (1d01c5f).
Report is 164 commits behind head on main.

Files with missing lines	Patch %	Lines
src/plugins/data/public/search/search_service.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #8368   +/-   ##
=======================================
  Coverage   60.92%   60.92%           
=======================================
  Files        3749     3749           
  Lines       89021    89021           
  Branches    13899    13899           
=======================================
  Hits        54236    54236           
  Misses      31423    31423           
  Partials     3362     3362           

Flag	Coverage Δ
Linux_1	28.85% <0.00%> (ø)
Linux_2	56.35% <ø> (ø)
Linux_3	37.76% <0.00%> (?)
Linux_4	29.94% <0.00%> (ø)
Windows_1	28.87% <0.00%> (ø)
Windows_2	56.30% <ø> (ø)
Windows_3	37.76% <0.00%> (-0.01%)	⬇️
Windows_4	29.94% <0.00%> (ø)

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

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

[ashwin-pc]

@AMoo-Miki @kavilla Can you review this?

@ruanyl Can you add details about why this is needed now?

[ashwin-pc]

@ruanyl i dont this the interceptors do that much anymore. afaik the ppl search interceptor should only add the time field to the ppl query from the query state and nothign more. SQL should make no modifications to the query. Is this not whats happening? I agree with you that the inerceptor should not modify anything in the query. I think the time selector is the only exception here but i could be swayed there as well.

If this is true, do we still need your change?

[ruanyl]

@ashwin-pc You're right, that's true for sql interceptor. For ppl interceptor it adds time filter even when there is a time filter in the ppl query, for example, I have ppl like source=opensearch_dashboards_sample_data_ecommerce | where order_date >= TIMESTAMPADD(DAY, -2, NOW()) | stats COUNT()

The actually query send becomes this which adds the time range from the ui

source = opensearch_dashboards_sample_data_ecommerce  | where `order_date` >= '2024-10-01 00:04:35' and `order_date` <= '2024-10-01 00:19:35' | where order_date >= TIMESTAMPADD(DAY, -2, NOW()) | stats COUNT()

And also it seems it modifies the aggregation which I don't fully understand the reason, maybe it's the default aggregation based on the time range?

        aggsConfig.qs = {
          [key]: `${query.query} | stats count() by span(${query.dataset!.timeFieldName}, ${
            value[aggTypeKey].fixed_interval ??
            value[aggTypeKey].calendar_interval ??
            this.aggsService.calculateAutoTimeExpression({
              from: fromDate,
              to: toDate,
              mode: 'absolute',
            })
          })`,
        };

If this is true, do we still need your change?

We still need this change because this change is about the 1st point for the language selector. The issue that I'm having is I'm using another search strategy, but after I selected PPL or SQL on the discover page, because of the following code, it will always use what's selected(ppl or sql) on discover app even I'm on another app. And actually, it calls data.search(request, {strategy: "whatever_strategy"}) directly and has nothing to do with the language selected on discover app.

      if (isEnhancedEnabled) {
        const queryStringManager = getQueryService().queryString;
        const language = queryStringManager.getQuery().language;
        const languageConfig = queryStringManager.getLanguageService().getLanguage(language);
        queryStringManager.getLanguageService().setUiOverridesByUserQueryLanguage(language);

        if (languageConfig) {
          return languageConfig.search.search(request, options);
        }
      }

[ashwin-pc]

I'll let @kavilla answer why we modify aggs. Based on my understanding, we shouldn't. Also adding the time filter even if we already have on in the query is intended behavior to be consistent. If the UI has a time filter, it makes more sense to apply it to the query than not because it simpler to reason that way. Optionally adding it to the query makes it harder to reason when the UI time filter is applied and when the query time filter is used.

As for the change you are making, it seems fine temporarily to unblock your usecase but long term the goal is to have a strategy per dataset type and a interceptor per language and this solution breaks that. @kavilla will do a post 2.18 review of the design since we'll have to consolidate all the recent changes we've been making to the search pipeline.

[joshuali925]

i briefly looked into this, the fix looks good, but how do we know the options.strategy is supposed to be used with the defaultSearchInteceptor but not the languageConfig.search interceptor?

[ashwin-pc]

That was my concern as well.

[ruanyl]

i briefly looked into this, the fix looks good, but how do we know the options.strategy is supposed to be used with the defaultSearchInteceptor but not the languageConfig.search interceptor?

@ashwin-pc @joshuali925
Thanks for following up :)

Both the current sql_search_interceptor and ppl_search_interceptor don't "care" the options.strategy, they have it's own logic to "compute" the strategy internally:
For sql_search_interceptor:

    let strategy = datasetType === DATASET.S3 ? SEARCH_STRATEGY.SQL_ASYNC : SEARCH_STRATEGY.SQL;

    if (datasetType) {
      const datasetTypeConfig = this.queryService.queryString
        .getDatasetService()
        .getType(datasetType);
      strategy = datasetTypeConfig?.getSearchOptions?.().strategy ?? strategy;
    }

For ppl_search_interceptor:

    let strategy = SEARCH_STRATEGY.PPL;

    if (datasetType) {
      const datasetTypeConfig = this.queryService.queryString
        .getDatasetService()
        .getType(datasetType);
      strategy = datasetTypeConfig?.getSearchOptions?.().strategy ?? strategy;
    }

So this PR won't affect the existing two languageConfig interceptors. However, I'd say this PR is just a temporary workaround to fix the issue that the passed strategy been ignored. I believe we would need a throughly refactor on this.

[joshuali925]

i'm ok with this temporary workaround. i think it would be good to have some comments to call out that the framework now assumes all search interceptors from language configs do not use options.strategy and do expect options.strategy to be absent

@kavilla could you also take a look since you are more familiar with this part