[ES|QL] Ignore drop commands for date histogram in discover by dej611 · Pull Request #171769 · elastic/kibana

dej611 · 2023-11-22T16:22:08Z

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.

Unit or functional tests were updated or added to match the most common scenarios

elasticmachine · 2023-11-23T13:53:38Z

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

elasticmachine · 2023-11-23T13:53:40Z

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

stratoula · 2023-11-23T14:39:56Z

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

…x/169907

dej611 · 2023-11-24T13:03:40Z

@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 · 2023-11-27T07:04:22Z

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.

stratoula · 2023-11-27T07:05:15Z

packages/kbn-unified-data-table/src/components/data_table_columns.tsx

+) {
  const timeFieldName = dataView.timeFieldName;

+  if (isPlainRecord) {


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

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

stratoula

Changes LGTM, this works exactly as I am expecting 👏

weltenwort · 2023-11-27T12:26:29Z

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 · 2023-11-27T12:29:59Z

packages/kbn-unified-data-table/src/components/data_table_columns.tsx

+    return showTimeCol;
+  }
+  // flattened is the _source content in this case
+  return rows.some((row) => timeFieldName in row.flattened);


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

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

Fixed with 4db4e3a

jughosta · 2023-11-27T12:32:14Z

packages/kbn-es-query/src/es_query/es_aggregate_query.ts

  return '';
 }
+
+export function cleanupESQLQuery(esql?: string): string {


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

Fixed with fc4c37f

dej611 · 2023-11-27T13:40:57Z

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

LGTM 👍

kibana-ci · 2023-11-27T15:02:28Z

💚 Build Succeeded

Buildkite Build
Commit: 4db4e3a

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

💚 Build #179027 succeeded 78fae0a
💚 Build #178983 succeeded 3e516da
💚 Build #178789 succeeded 982d33b
💛 Build #178348 was flaky 9711de8
💔 Build #178125 failed ab3b740

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

🐛 ignore drop statements for histogram

ab3b740

Merge branch 'main' into fix/169907

9711de8

dej611 marked this pull request as ready for review November 23, 2023 13:53

dej611 requested a review from a team as a code owner November 23, 2023 13:53

stratoula self-requested a review November 23, 2023 14:07

dej611 added 4 commits November 24, 2023 11:45

Merge remote-tracking branch 'upstream/main' into fix/169907

132fd0f

Merge remote-tracking branch 'upstream/main' into fix/169907

0518db3

🐛 Do not extends with timeField if the dataView is text based

c47ab3f

Merge branch 'fix/169907' of https://github.com/dej611/kibana into fi…

982d33b

…x/169907

Merge branch 'main' into fix/169907

3e516da

stratoula reviewed Nov 27, 2023

View reviewed changes

🐛 Hide timeField column only in the drop specific case

78fae0a

stratoula approved these changes Nov 27, 2023

View reviewed changes

dej611 enabled auto-merge (squash) November 27, 2023 11:23

jughosta reviewed Nov 27, 2023

View reviewed changes

dej611 added 2 commits November 27, 2023 14:50

👌 Integrate feedback for fn name

fc4c37f

👌 Integrate feedback on fast lookups

4db4e3a

jughosta approved these changes Nov 27, 2023

View reviewed changes

dej611 merged commit d900f84 into elastic:main Nov 27, 2023

kibanamachine added the backport:skip This PR does not require backporting label Nov 27, 2023

Conversation

dej611 commented Nov 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

elasticmachine commented Nov 23, 2023

Uh oh!

elasticmachine commented Nov 23, 2023

Uh oh!

stratoula commented Nov 23, 2023

Uh oh!

dej611 commented Nov 24, 2023

Uh oh!

stratoula commented Nov 27, 2023

Uh oh!

stratoula Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

dej611 Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

stratoula left a comment

Choose a reason for hiding this comment

Uh oh!

weltenwort commented Nov 27, 2023

Uh oh!

jughosta Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

jughosta Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

dej611 Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

jughosta Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

dej611 Nov 27, 2023

Choose a reason for hiding this comment

Uh oh!

dej611 commented Nov 27, 2023

Uh oh!

jughosta left a comment

Choose a reason for hiding this comment

Uh oh!

kibana-ci commented Nov 27, 2023

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Async chunks

Page load bundle

API count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dej611 commented Nov 22, 2023 •

edited

Loading