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

(dsl): Support Disjunction max query #360

Merged

Conversation

vanjaftn
Copy link
Contributor

@vanjaftn vanjaftn commented Nov 10, 2023

Part of #91

@vanjaftn vanjaftn changed the title (dsl): Support Disjunction max query (dsl): Support Disjunction max query Nov 10, 2023
* an instance of [[zio.elasticsearch.query.DisjunctionMax]] that represents the `disjunction max` query to be
* performed.
*/
final def disjunctionMax[S](queries: Chunk[ElasticQuery[S]]): DisjunctionMaxQuery[S] =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you implement raw method also? You can look at bool query as a reference (filter methods for example)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, check if [S: Schema] is required.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can there be zero queries? If so, you can use ElasticQuery[S]* maybe.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that there must be at least one query, as it is a required field.

* Constructs an instance of [[zio.elasticsearch.query.DisjunctionMax]] using the specified parameters.
*
* @param queries
* the type-safe [[ElasticQuery]] object to be wrapped inside of disjunction max query
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are more than one query.

Comment on lines 209 to 227
* @param allQueries
* the queries to be added
* @tparam S1
* the type of the sub-queries, for which an implicit [[zio.schema.Schema]] is required
* @return
* an instance of the [[zio.elasticsearch.query.DisjunctionMaxQuery]] with queries added.
*/
def queries[S1 <: S: Schema](allQueries: ElasticQuery[S1]*): DisjunctionMaxQuery[S1]

/**
* Adds specified queries to the [[zio.elasticsearch.query.DisjunctionMaxQuery]]. Returned documents must match one or
* more of these queries.
*
* @param allQueries
* the queries to be added
* @return
* an instance of the [[zio.elasticsearch.query.DisjunctionMaxQuery]] with queries added.
*/
def queries(allQueries: ElasticQuery[Any]*): DisjunctionMaxQuery[S]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can remove these two methods, as we already ask user for queries within the creation of query.

* @param value
* a number to set `tieBreaker` parameter to
* @return
* a new instance of the [[zio.elasticsearch.query.DisjunctionMaxQuery]] with the `tieBreaker` value set.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* a new instance of the [[zio.elasticsearch.query.DisjunctionMaxQuery]] with the `tieBreaker` value set.
* an instance of the [[zio.elasticsearch.query.DisjunctionMaxQuery]] enriched with the `tieBreaker` parameter.

title: "Disjunction max Query"
---

The `Disjunction max` query returns documents that match one or more query clauses. For documents that match multiple query clauses, the relevance score is set to the highest relevance score from all matching query clauses. When the relevance scores of the returned documents are identical, tie breaker parameter gives more weight to documents that match multiple query clauses.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The `Disjunction max` query returns documents that match one or more query clauses. For documents that match multiple query clauses, the relevance score is set to the highest relevance score from all matching query clauses. When the relevance scores of the returned documents are identical, tie breaker parameter gives more weight to documents that match multiple query clauses.
The `Disjunction max` query returns documents that match one or more query clauses. For documents that match multiple query clauses, the relevance score is set to the highest relevance score from all matching query clauses. When the relevance scores of the returned documents are identical, tie breaker parameter can be used for giving more weight to documents that match multiple query clauses.

Comment on lines 14 to 22
You can create a `Disjunction max` query using the `disjunctionMax` method this way:
```scala
val query: DisjunctionMaxQuery = disjunctionMax( queries = Chunk( term( field = "stringField", value = "test"), exists( field = "intField")))
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
You can create a `Disjunction max` query using the `disjunctionMax` method this way:
```scala
val query: DisjunctionMaxQuery = disjunctionMax( queries = Chunk( term( field = "stringField", value = "test"), exists( field = "intField")))
```
You can create a `Disjunction max` query using the `disjunctionMax` method this way:
```scala
val query: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(term(field = "stringField", value = "test"), exists(field = "intField")))

Copy link
Collaborator

Choose a reason for hiding this comment

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

When you update public api for this query, update this also.

Comment on lines 20 to 27
```scala
val queryWithPrefixLength: DisjunctionMaxQuery = disjunctionMax( queries = Chunk( queries = Chunk( exists("existsField"), ids("1", "2", "3"))).tieBreaker(0.5f))
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
```scala
val queryWithPrefixLength: DisjunctionMaxQuery = disjunctionMax( queries = Chunk( queries = Chunk( exists("existsField"), ids("1", "2", "3"))).tieBreaker(0.5f))
```
```scala
val queryWithPrefixLength: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(queries = Chunk(exists("existsField"), ids("1", "2", "3"))).tieBreaker(0.5f))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, something seems off with these Chunks.

firstDocument.copy(stringField = s"this is a ${firstDocument.stringField} test.")
secondDocumentUpdated =
secondDocument.copy(stringField =
s"this is not a ${firstDocument.stringField} test. It is a ${secondDocument.stringField} test, but not ${firstDocument.stringField}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you put something more meaningful here?

@vanjaftn vanjaftn force-pushed the task-support-disjunction-max-query branch from f04af37 to 5f14e97 Compare November 13, 2023 09:50
Comment on lines 101 to 102
final def disjunctionMax[S: Schema](queries: Chunk[ElasticQuery[S]]): DisjunctionMaxQuery[S] =
DisjunctionMax[S](queries = queries, tieBreaker = None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use this instead: queries: ElasticQuery[S]*?

Comment on lines 113 to 114
final def disjunctionMax(queries: Chunk[ElasticQuery[Any]]): DisjunctionMaxQuery[Any] =
DisjunctionMax[Any](queries = queries, tieBreaker = None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you use this instead: queries: ElasticQuery[Any]*?


You can create a `Disjunction max` query using the `disjunctionMax` method this way:
```scala
val query: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(term(field = "stringField", value = "test"), exists( field = "intField")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
val query: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(term(field = "stringField", value = "test"), exists( field = "intField")))
val query: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(term(field = "stringField", value = "test"), exists(field = "intField")))

Comment on lines 21 to 27
val queryWithPrefixLength: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(exists("existsField"), ids("1", "2", "3"))).tieBreaker(0.5f)
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
val queryWithPrefixLength: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(exists("existsField"), ids("1", "2", "3"))).tieBreaker(0.5f)
```
val queryWithTieBreaker: DisjunctionMaxQuery = disjunctionMax(queries = Chunk(exists("existsField"), ids("1", "2", "3"))).tieBreaker(0.5f)

```

You can find more information about `Disjunction max` query [here](https://www.elastic.co/guide/en/elasticsearch/reference/current/query-dsl-dis-max-query.html).

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there one or two empty lines at the end? If there is two, omit one please.

* Constructs a type-safe instance of [[zio.elasticsearch.query.DisjunctionMax]] using the specified parameters.
*
* @param queries
* the type-safe [[ElasticQuery]] objects to be wrapped inside of disjunction max query
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* the type-safe [[ElasticQuery]] objects to be wrapped inside of disjunction max query
* a list of queries to be wrapped inside of disjunction max query

* Constructs an instance of [[zio.elasticsearch.query.DisjunctionMax]] using the specified parameters.
*
* @param queries
* the [[ElasticQuery]] objects to be wrapped inside of disjunction max query
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* the [[ElasticQuery]] objects to be wrapped inside of disjunction max query
* a list of queries to be wrapped inside of disjunction max query

@vanjaftn vanjaftn force-pushed the task-support-disjunction-max-query branch 2 times, most recently from d680a2f to 98c8137 Compare November 15, 2023 14:42
* performed.
*/
final def disjunctionMax[S: Schema](query: ElasticQuery[S], queries: ElasticQuery[S]*): DisjunctionMaxQuery[S] =
DisjunctionMax[S](query = query, queries = Chunk.fromIterable(queries), tieBreaker = None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you merge query and queries here?
queries = Chunk.fromIterable(queries).prepended(query) or something like this?

Copy link
Collaborator

Choose a reason for hiding this comment

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

So then we will not have both query and queries in DisjunctionMax.

* performed.
*/
final def disjunctionMax(query: ElasticQuery[Any], queries: ElasticQuery[Any]*): DisjunctionMaxQuery[Any] =
DisjunctionMax[Any](query = query, queries = Chunk.fromIterable(queries), tieBreaker = None)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here.

query: ElasticQuery[S],
queries: Chunk[ElasticQuery[S]],
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you apply change I have asked, then we will have only queries here.

@vanjaftn vanjaftn force-pushed the task-support-disjunction-max-query branch from 3deeabe to b04fca9 Compare November 17, 2023 11:21
@dbulaja98 dbulaja98 merged commit fa1f343 into lambdaworks:main Nov 17, 2023
13 checks passed
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.

None yet

2 participants