-
Notifications
You must be signed in to change notification settings - Fork 919
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
Discover Bug Fixes #7948
Discover Bug Fixes #7948
Conversation
❌ Empty Changelog SectionThe 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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7948 +/- ##
==========================================
- Coverage 60.97% 60.97% -0.01%
==========================================
Files 3684 3684
Lines 87038 87047 +9
Branches 13376 13378 +2
==========================================
+ Hits 53074 53078 +4
- Misses 30750 30754 +4
- Partials 3214 3215 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
47331f8
to
75f5bed
Compare
Signed-off-by: Suchit Sahoo <[email protected]>
Signed-off-by: Suchit Sahoo <[email protected]>
75f5bed
to
a8cb534
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for helping to fix these bugs!
@@ -40,7 +40,7 @@ export class Facet { | |||
const query: Query = request.body.query; | |||
const { format, df } = request.body; | |||
const params = { | |||
body: { ...query }, | |||
body: { query: query.query }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we removed the rest of the query object? I don't remember if the fetch()
function for PPL
and SQL
use the rest of the object off the top of my head.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before this change the request body that we used to send was of the following format
{
query: {
query: 'SELECT * FROM aaa* LIMIT 10',
language: 'SQL',
dataset: [Object],
format: 'jdbc'
},
aggConfig: null,
df: null
}
However the SQL plugins request only needs the following parameters.
Also we observed sending in the extra unrecognized parameters was causing bugs where the SQL Search not working for IndexPatterns with Wildcards in the name, therefore this PR aims to cleanup the request body that we send.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah end up doing something similar here too https://github.com/opensearch-project/OpenSearch-Dashboards/pull/7943/files
* Fixed SQL search for Wildcard IndexPattern Signed-off-by: Suchit Sahoo <[email protected]> * Fix Discover intial pageload for non DQL language Signed-off-by: Suchit Sahoo <[email protected]> --------- Signed-off-by: Suchit Sahoo <[email protected]> (cherry picked from commit c7843fe) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* Fixed SQL search for Wildcard IndexPattern * Fix Discover intial pageload for non DQL language --------- (cherry picked from commit c7843fe) Signed-off-by: Suchit Sahoo <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Description
This changes fixes the following bugs
Issues Resolved
Screenshot
Meeting.Recording.-.Sahoo.Suchit.Instant.Meeting.6.mp4
Testing the changes
Changelog
Check List
yarn test:jest
yarn test:jest_integration