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

Add missing fields to ES compatible Query API #5098

Merged
merged 2 commits into from
Jun 12, 2024
Merged

Add missing fields to ES compatible Query API #5098

merged 2 commits into from
Jun 12, 2024

Conversation

kuzaxak
Copy link
Collaborator

@kuzaxak kuzaxak commented Jun 9, 2024

Description

I've added additional fields as serde_json::Value to prevent complex struct definitions as we won't use them anyway.

Addressing #5097

How was this PR tested?

I've tested it locally using queries taken from OSD

Additionally I've added some tests for multi_match parser code.

@kuzaxak kuzaxak requested a review from fulmicoton June 9, 2024 08:07
kuzaxak added 2 commits June 9, 2024 12:12
* [_source][1] schema
* [docvalue][2] schema

I've added additional fields as `serde_json::Value` to prevent complex
struct definitions as we won't use them anyway.

[1]: https://www.elastic.co/guide/en/elasticsearch/reference/7.17/search-fields.html#source-filtering
[2]: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-fields.html#docvalue-fields
According to [doc][1] multi_match has 6 modes. Best fields and Cross
fields can be converted to Most fields as they close by logic and I
couldn't find their implementations.

BoolPrefix was implemented and I added it to the parser.

Tests was extended to cover all cases, for that I changed a bit of
structures and added missing traits implementations.

[1]: https://opensearch.org/docs/latest/query-dsl/full-text/multi-match/#multi-match-query-types
@@ -40,6 +40,8 @@ pub struct RangeQueryParams {
lte: Option<JsonLiteral>,
#[serde(default)]
boost: Option<NotNaNf32>,
#[serde(default)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#[serde(default)]
// Currently NO-OP (see #5098)
#[serde(default)]

@fulmicoton fulmicoton merged commit 379a752 into quickwit-oss:main Jun 12, 2024
5 checks passed
@fulmicoton
Copy link
Contributor

@kuzaxak to make things smoother I merged and will commit my comments above that. Next time, could you create a feature branch on the quickwit repo (I think you have the permissions to do that)

@fulmicoton
Copy link
Contributor

For info: #5111

@kuzaxak
Copy link
Collaborator Author

kuzaxak commented Jun 12, 2024

@kuzaxak to make things smoother I merged and will commit my comments above that. Next time, could you create a feature branch on the quickwit repo (I think you have the permissions to do that)

@fulmicoton
Could you please elaborate on what you mean by the feature branch? Create branch and point all related PRs to it and then one more Feature PR to merge it to the main one?

@fulmicoton
Copy link
Contributor

I mean pushing a branch on quickwit-oss/quickwit.

@kuzaxak kuzaxak deleted the add_missing_fields_to_es_query branch June 12, 2024 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants