Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[ES|QL] Ignore drop commands for date histogram in discover #171769

Merged
merged 10 commits into from
Nov 27, 2023
Merged

[ES|QL] Ignore drop commands for date histogram in discover #171769

merged 10 commits into from
Nov 27, 2023

Conversation

dej611
Copy link
Contributor

@dej611 dej611 commented Nov 22, 2023 •
edited
Loading

Summary

Fixes #169907

This PR cleans the ES|QL statement from DROP commands before sending it over for the date histogram chart in Lens.

Checklist

Delete any items that are not applicable to this PR.

@dej611 dej611 added release_note:fix Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Lens Feature:ES|QL ES|QL related features in Kibana v8.12.0 Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. labels Nov 22, 2023
@dej611 dej611 marked this pull request as ready for review November 23, 2023 13:53
@dej611 dej611 requested a review from a team as a code owner November 23, 2023 13:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

@stratoula stratoula self-requested a review November 23, 2023 14:07
@stratoula
Copy link
Contributor

@dej611 looks great. How easy is to not display the @timestamp column in the datatable?

image

@dej611
Copy link
Contributor Author

dej611 commented Nov 24, 2023

@dej611 looks great. How easy is to not display the @timestamp column in the datatable?

Fixed with c47ab3f .
I believe the fix will be temporary as there's a root problem with the dataView which does not get updated with the ES|QL query, together with the @timestamp detection in the unified search. I see how an "extended" dataView can be useful here but perhaps it is worth a discussion to define some boundaries.

@stratoula
Copy link
Contributor

We lost the @timestamp column here and we dont want it. We want the @timestamp column to disappear when the user drops the @timestamp if possible not always.

image

stratoula
stratoula reviewed Nov 27, 2023
View reviewed changes
packages/kbn-unified-data-table/src/components/data_table_columns.tsx Outdated
const timeFieldName = dataView.timeFieldName;

if (isPlainRecord) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hides the @timestamp column all the time. We don't want that. We just want to hide it when the user is dropping the field

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 78fae0a added also a full text coverage for the new utility function.

stratoula
stratoula approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM, this works exactly as I am expecting 👏

@dej611 dej611 enabled auto-merge (squash) November 27, 2023 11:23
@weltenwort
Copy link
Member

Please excuse my naive and uninvited question, but we're also consumers of the unified histogram. 😇

Is this string manipulation safe without parsing the ES|QL AST? What happens to a query like FROM es-audit-logs | WHERE query LIKE '*| DROP @timestamp*'? And are there other ES|QL operations that need to be guarded against?

jughosta
jughosta reviewed Nov 27, 2023
View reviewed changes
packages/kbn-unified-data-table/src/components/data_table_columns.tsx Outdated
return showTimeCol;
}
// flattened is the _source content in this case
return rows.some((row) => timeFieldName in row.flattened);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Iterating through all results can be a long operation. Can we avoid it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example, we could search in the returned ESQL fields. UnifiedDataTable accesses them via columnTypes. So in your case it could be:

return Boolean(columnTypes[timeFieldName])

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with 4db4e3a

packages/kbn-es-query/src/es_query/es_aggregate_query.ts Outdated
@@ -66,3 +66,8 @@ export function getIndexPatternFromESQLQuery(esql?: string): string {
}
return '';
}

export function cleanupESQLQuery(esql?: string): string {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be great to describe the purpose of the clean up. Or at least change the function name to something like cleanupESQLQueryForLensSuggestions

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with fc4c37f

@dej611
Copy link
Contributor Author

dej611 commented Nov 27, 2023

Please excuse my naive and uninvited question, but we're also consumers of the unified histogram. 😇

Is this string manipulation safe without parsing the ES|QL AST? What happens to a query like FROM es-audit-logs | WHERE query LIKE '*| DROP @timestamp*'? And are there other ES|QL operations that need to be guarded against?

This is more a general issue and probably with #170071 will be possible to perform query manipulation like that. For now that ES|QL query will break as there's often the basic assumption that commands are separated by | (query.split('|') in lots of places...).

jughosta
jughosta approved these changes Nov 27, 2023
View reviewed changes
Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
@kbn/es-query 199 201 +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
cloudSecurityPosture 401.3KB 401.5KB +147.0B
discover 589.3KB 589.4KB +149.0B
unifiedHistogram 50.6KB 50.6KB +47.0B
total +343.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kbnUiSharedDeps-srcJs 2.2MB 2.2MB +150.0B
Unknown metric groups

API count

id before after diff
@kbn/es-query 259 261 +2

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@dej611 dej611 merged commit d900f84 into elastic:main Nov 27, 2023
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Nov 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stratoula stratoula stratoula approved these changes

@jughosta jughosta jughosta approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting Feature:ES|QL ES|QL related features in Kibana Feature:Lens release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. Team:Visualizations Visualization editors, elastic-charts and infrastructure v8.12.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[ES|QL][Discover] Dropping the @timestamp returns errors
7 participants
@dej611 @elasticmachine @stratoula @weltenwort @kibana-ci @jughosta @kibanamachine