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.

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

[ES|QL] Fixes the problem with Discover and queries without the from command #180692

Merged
merged 3 commits into from
Apr 13, 2024

Conversation

stratoula
Copy link
Contributor

@stratoula stratoula commented Apr 12, 2024

Summary

Closes #163417

When there was no from command in the query we were using the current dataview. This might have the @timestamp field which is not returned by the ROW ... or Show meta commands. So the histogram was failing.

I am solving this issue by creating a dataview based on the current dataview but without the timeFieldName

image

I still think we should find another way to deal with these commands but for now this is a nice way forward

Before:

image

Checklist

@stratoula
Copy link
Contributor Author

/ci

@kertal
Copy link
Member

kertal commented Apr 12, 2024

let's be sure to align, since now we have 2 similar solutions #179020

@stratoula
Copy link
Contributor Author

stratoula commented Apr 12, 2024

I didn't see Matt's PR tbh, I think it will still have the @timestamp error I mention here

It seems that it was created 3 weeks ago but didnt finish. Is there any particular reason?

It seems that we are at the same track though, I am also fixing the aforementioned bug though. Is there any problem on moving on with this PR?

I am expecting multiple users to use the ROW command, I don't want to have this bug before GA.

Update: On. second glance without testing his PR, I think he doesnt have the bug either. So let's proceed with one of the PRs but please let's merge it on 8.14 🙏 (I like my extra check, why search on the fields when we know that for empty string it won't have the field?)

@kertal
Copy link
Member

kertal commented Apr 12, 2024

Update: On. second glance without testing his PR, I think he doesnt have the bug either. So let's proceed with one of the PRs but please let's merge it on 8.14 🙏 (I like my extra check, why search on the fields when we know that for empty string it won't have the field?)

yes, I've tested with both PRs and both fix the problem 🎉 ... the extra check makes sense IMO, but can also be added to the other PR for sure

@stratoula stratoula added Feature:Discover Discover Application release_note:fix backport:skip This commit does not require backporting Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.14.0 labels Apr 12, 2024
@stratoula stratoula marked this pull request as ready for review April 12, 2024 14:29
@stratoula stratoula requested a review from a team as a code owner April 12, 2024 14:29
@elasticmachine
Copy link
Contributor

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

@stratoula
Copy link
Contributor Author

I am a bit confused with this

but can also be added to the other PR for sure

I am opening the PR for review!

@mattkime
Copy link
Contributor

@stratoula Go ahead and grab the functional test from my PR - #179020

@stratoula
Copy link
Contributor Author

/ci

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Async chunks

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

id before after diff
discover 637.2KB 637.1KB -14.0B

History

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

Copy link
Contributor

@mattkime mattkime left a comment

Choose a reason for hiding this comment

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

changes look good and work well!

@stratoula stratoula merged commit fd9722c into elastic:main Apr 13, 2024
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Discover Discover Application release_note:fix Team:DataDiscovery Discover, search (e.g. data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. v8.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ESQL] Support commands which dont need an index
5 participants