-
Notifications
You must be signed in to change notification settings - Fork 21
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
Apply ad-hoc filters to PPL queries before sending it to the backend #244
Apply ad-hoc filters to PPL queries before sending it to the backend #244
Conversation
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.
Approved with a question about ppl queries
{ key: 'count', operator: '<', value: 100, condition: '' }, | ||
{ key: 'timestamp', operator: '=', value: '2020-11-22 16:40:43', condition: '' }, | ||
]; | ||
it('should return query with ad-hoc filters applied', () => { |
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.
Should this also have the query is empty case or is that not possible for ppl queries?
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.
right now the flow of the PPL execution is to always make sure the query string is not empty by adding the index that is being searched to the query string if the query string is empty
opensearch-datasource/src/datasource.ts
Lines 741 to 743 in 6e6f9af
if (!queryString) { | |
queryString = `source=\`${this.indexPattern.getPPLIndexPattern()}\``; | |
} |
this happens happens before ad-hoc filters are added to the query string, so for this PR, i'm just testing the adding of the ad-hoc filters.
an empty query string test case will probably be more suited for #200
in short, ppl queries should not be empty, but that should be handled outside of the addAdHocFilters
code.
{ key: 'a', operator: '!~', value: '', condition: '' }, | ||
]); | ||
expect(query).toBe('source = test-index'); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('with 1 ad hoc filter', () => { |
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.
just curious - any reason why only testing Lucene in this test case?
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.
i didn't think there was anything that could be added under this test case with 1 ad hoc filter for PPL queries that couldn't be covered by the multiple ad hoc filters test cases.
ef2a561
to
0beb911
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.
Nice!
What this PR does / why we need it:
Prerequisite work for migrating PPL queries to be executed on the backend. Ad-hoc filters are applied on the frontend only so this work ensures they're added before sending a query to the backend.
Which issue(s) this PR fixes:
Fixes #226
Special notes for your reviewer:
You currently can't test this except for running the unit tests since no PPL queries have been migrated to execute on the backend yet.