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 Terms set query #355

Merged
merged 8 commits into from
Nov 14, 2023

Conversation

vanjaftn
Copy link
Contributor

@vanjaftn vanjaftn commented Nov 6, 2023

Part of #91

@dbulaja98 dbulaja98 changed the title (dsl): Support Terms Set query (dsl): Support Terms set query Nov 7, 2023
title: "Terms Set Query"
---

The `TermsSet` query returns documents that contain the minimum amount of exact terms in a provided field. The terms set query is the same as [[zio.elasticsearch.query.TermsQuery]], except you can define the number of matching terms required to return a document.
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 `TermsSet` query returns documents that contain the minimum amount of exact terms in a provided field. The terms set query is the same as [[zio.elasticsearch.query.TermsQuery]], except you can define the number of matching terms required to return a document.
The `TermsSet` query returns documents that contain the minimum amount of exact terms in a provided field. The Terms set query is the same as [[zio.elasticsearch.query.TermsQuery]], except you can define the number of matching terms required to return a document.

@@ -936,7 +936,7 @@ object ElasticQuery {
* @tparam A
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 A before S in scaladoc?

final def termsSet[S, A: ElasticPrimitive](
field: Field[S, A],
minimumShouldMatchField: String,
terms: A*
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 terms?

*/
final def termsSetScript[A: ElasticPrimitive](
field: String,
minimumShouldMatchScript: zio.elasticsearch.script.Script,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why with prefix?

@@ -1477,7 +1551,7 @@ object HttpExecutorSpec extends IntegrationSpec {
Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
),
test("search for a document using script query") {
test("search for a document using should without satisfying minimumShouldMatch condition") {
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 omit: without satisfying minimumShouldMatch condition? If that makes sense.

@@ -1342,6 +1342,80 @@ object HttpExecutorSpec extends IntegrationSpec {
Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
),
test("search for a document using a terms set query with minimumShouldMatchField") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe put 2 in intField, and make one of them not passing the check.

Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
),
test("search for a document using a terms set query with minimumShouldMatchScript") {
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.


You can create a [type-safe](https://lambdaworks.github.io/zio-elasticsearch/overview/overview_zio_prelude_schema) `TermsSet` query with defined `minimumShouldMatchField` using the `termsSet` method this way:
```scala
val query: TermsSetQuery = termsSet(field = Document.name, minimumShouldMatchField = "intField", terms = 1, 2, 3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Check if minimumShouldMatchField can be type-safe too.

Comment on lines 25 to 26
```scala
val query: TermsSetQuery = termsSetScript(field = "stringField", minimumShouldMatchScript = Script("doc['intField'].value"), terms = 1, 2, 3)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put Script import here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also because of stringField, consider changing 1,2,3 to something else (everywhere here)

Comment on lines 21 to 22
val query: TermsSetQuery = termsSet(field = Document.name, minimumShouldMatchField = Document.field, terms = "a", "b", "c")
```
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: TermsSetQuery = termsSet(field = Document.name, minimumShouldMatchField = Document.field, terms = "a", "b", "c")
```
val query: TermsSetQuery = termsSet(field = Document.name, minimumShouldMatchField = Document.intField, terms = "a", "b", "c")

terms = Chunk.fromIterable(terms),
minimumShouldMatchField = Some(minimumShouldMatchField),
minimumShouldMatchScript = None,
boost = None
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put boost before minimumShouldMatchField and minimumShouldMatchScript.

terms: Chunk[A],
minimumShouldMatchField: Option[String],
minimumShouldMatchScript: Option[zio.elasticsearch.script.Script],
boost: Option[Double]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Put boost before minimumShouldMatchField an minimumShouldMatchScript.

Comment on lines 40 to 41
val queryWithBoost: TermsSetQuery = termsSet(field = "booleanField", minimumShouldMatchField = "intField", terms = true, false).boost(2.0)
val queryWithBoost: TermsSetQuery = termsSetScript(field = "booleanField", minimumShouldMatchScript = Script("doc['intField'].value"), terms = true, false).boost(2.0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Give them different names.

Comment on lines +1681 to +1683
terms = "a",
"b",
"c"
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 this all in the same line?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same question for every situation like this in new tests you have added.

Comment on lines +1665 to +1667
terms = "a",
"b",
"c"
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 this all in the same line?

@dbulaja98 dbulaja98 merged commit dbeb4b0 into lambdaworks:main Nov 14, 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