Skip to content

Commit

Permalink
Make the drop_null_columns configurable in ES|QL search strategies (e…
Browse files Browse the repository at this point in the history
…lastic#176236)

## Summary

Fixes elastic#176227

As not everyone is using expressions and the `?drop_null_columns`
significantly changing the output, I am making this configurable. So for
the expressions users it will always be true but for anyone else it
would be false unless they specifically configure it.

This PR is fixing it but Nathan is going to follow up:

- maps are setting the drop_null_columns to true (as it makes the
response smaller, which is always a benefit)
- add some FTs to test the output

---------

Co-authored-by: Julia Rechkunova <[email protected]>
  • Loading branch information
2 people authored and CoenWarmer committed Feb 15, 2024
1 parent 56d75ec commit d6f57e0
Show file tree
Hide file tree
Showing 6 changed files with 25 additions and 11 deletions.
1 change: 1 addition & 0 deletions packages/kbn-es-types/src/search.ts
Original file line number Diff line number Diff line change
Expand Up @@ -678,4 +678,5 @@ export interface ESQLSearchParams {
query: string;
filter?: unknown;
locale?: string;
dropNullColumns?: boolean;
}
5 changes: 4 additions & 1 deletion src/plugins/data/common/search/expressions/esql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -194,7 +194,10 @@ export const getEsqlFn = ({ getStartDependencies }: EsqlFnArguments) => {
return search<
IKibanaSearchRequest<ESQLSearchParams>,
IKibanaSearchResponse<ESQLSearchReponse>
>({ params }, { abortSignal, strategy: ESQL_ASYNC_SEARCH_STRATEGY }).pipe(
>(
{ params: { ...params, dropNullColumns: true } },
{ abortSignal, strategy: ESQL_ASYNC_SEARCH_STRATEGY }
).pipe(
catchError((error) => {
if (!error.attributes) {
error.message = `Unexpected error from Elasticsearch: ${error.message}`;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ describe('ES|QL async search strategy', () => {
it('sets transport options on POST requests', async () => {
const transportOptions = { maxRetries: 1 };
mockApiCaller.mockResolvedValueOnce(mockAsyncResponse);
const params = {};
const params = { query: 'from logs' };
const esSearch = esqlAsyncSearchStrategyProvider(mockSearchConfig, mockLogger);

await firstValueFrom(
Expand All @@ -139,6 +139,7 @@ describe('ES|QL async search strategy', () => {
keep_alive: '60000ms',
wait_for_completion_timeout: '100ms',
keep_on_completion: false,
query: 'from logs',
},
}),
expect.objectContaining({ maxRetries: 1, meta: true, signal: undefined })
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { catchError, tap } from 'rxjs/operators';
import { getKbnServerError } from '@kbn/kibana-utils-plugin/server';
import { SqlQueryRequest } from '@elastic/elasticsearch/lib/api/typesWithBodyKey';
import { SqlGetAsyncResponse } from '@elastic/elasticsearch/lib/api/types';
import type { ESQLSearchParams } from '@kbn/es-types';
import {
getCommonDefaultAsyncSubmitParams,
getCommonDefaultAsyncGetParams,
Expand All @@ -22,12 +23,18 @@ import { IKibanaSearchRequest, IKibanaSearchResponse, pollSearch } from '../../.
import { toAsyncKibanaSearchResponse } from './response_utils';
import { SearchConfigSchema } from '../../../../config';

// `drop_null_columns` is going to change the response
// now we get `all_columns` and `columns`
// `columns` contain only columns with data
// `all_columns` contain everything
type ESQLQueryRequest = ESQLSearchParams & SqlQueryRequest['body'];

export const esqlAsyncSearchStrategyProvider = (
searchConfig: SearchConfigSchema,
logger: Logger,
useInternalUser: boolean = false
): ISearchStrategy<
IKibanaSearchRequest<SqlQueryRequest['body']>,
IKibanaSearchRequest<ESQLQueryRequest>,
IKibanaSearchResponse<SqlGetAsyncResponse>
> => {
function cancelAsyncSearch(id: string, esClient: IScopedClusterClient) {
Expand All @@ -46,12 +53,12 @@ export const esqlAsyncSearchStrategyProvider = (
}

function asyncSearch(
{ id, ...request }: IKibanaSearchRequest<SqlQueryRequest['body']>,
{ id, ...request }: IKibanaSearchRequest<ESQLQueryRequest>,
options: IAsyncSearchOptions,
{ esClient, uiSettingsClient }: SearchStrategyDependencies
) {
const client = useInternalUser ? esClient.asInternalUser : esClient.asCurrentUser;

const { dropNullColumns, ...requestParams } = request.params ?? {};
const search = async () => {
const params = id
? {
Expand All @@ -63,7 +70,7 @@ export const esqlAsyncSearchStrategyProvider = (
}
: {
...(await getCommonDefaultAsyncSubmitParams(searchConfig, options)),
...request.params,
...requestParams,
};
const { body, headers, meta } = id
? await client.transport.request<SqlGetAsyncResponse>(
Expand All @@ -79,7 +86,7 @@ export const esqlAsyncSearchStrategyProvider = (
method: 'POST',
path: `/_query/async`,
body: params,
querystring: 'drop_null_columns',
querystring: dropNullColumns ? 'drop_null_columns' : '',
},
{ ...options.transport, signal: options.abortSignal, meta: true }
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,16 @@ export const esqlSearchStrategyProvider = (

const search = async () => {
try {
const { terminateAfter, ...requestParams } = request.params ?? {};
// `drop_null_columns` is going to change the response
// now we get `all_columns` and `columns`
// `columns` contain only columns with data
// `all_columns` contain everything
const { terminateAfter, dropNullColumns, ...requestParams } = request.params ?? {};
const { headers, body, meta } = await esClient.asCurrentUser.transport.request(
{
method: 'POST',
path: `/_query`,
querystring: 'drop_null_columns',
querystring: dropNullColumns ? 'drop_null_columns' : '',
body: {
...requestParams,
},
Expand Down
2 changes: 0 additions & 2 deletions test/api_integration/apis/search/bsearch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,6 @@ export default function ({ getService }: FtrProviderContext) {
expect(jsonBody[0].result.requestParams).to.eql({
method: 'POST',
path: '/_query',
querystring: 'drop_null_columns',
});
});

Expand Down Expand Up @@ -457,7 +456,6 @@ export default function ({ getService }: FtrProviderContext) {
expect(jsonBody[0].error.attributes.requestParams).to.eql({
method: 'POST',
path: '/_query',
querystring: 'drop_null_columns',
});
});
});
Expand Down

0 comments on commit d6f57e0

Please sign in to comment.