(dsl): Support highlights #141

markaya · 2023-03-31T12:34:26Z

No description provided.

modules/library/src/main/scala/zio/elasticsearch/ElasticHighlight.scala

drmarjanovic · 2023-03-31T12:43:03Z

modules/library/src/main/scala/zio/elasticsearch/ElasticHighlight.scala

+  def highlight[S](field: Field[S, _], fieldConfig: Map[String, Json]): Highlights =
+    Highlights(Chunk(HighlightField(field.toString, fieldConfig)))
+
+  def highlight(field: String, fieldConfig: Map[String, Json] = Map.empty): Highlights =


Can we use config instead of fieldConfig?

drmarjanovic · 2023-03-31T12:43:59Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

@@ -69,10 +70,17 @@ object ElasticRequest {
    GetById(index = index, id = id, refresh = None, routing = None)

  def search(index: IndexName, query: ElasticQuery[_]): SearchRequest =
-    Search(index = index, query = query, sortBy = Set.empty, routing = None)
+    Search(index = index, query = query, sortBy = Set.empty, routing = None, highlights = None)


Let's put this before routing. Let's use sorted params.

drmarjanovic · 2023-03-31T12:44:04Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

+      aggregation = aggregation,
+      sortBy = Set.empty,
+      routing = None,
+      highlights = None


drmarjanovic · 2023-03-31T12:45:05Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

@@ -311,16 +319,29 @@ object ElasticRequest {
      with HasRouting[SearchRequest]
      with WithSort[SearchRequest] {
    def aggregate(aggregation: ElasticAggregation): SearchAndAggregateRequest
+    def highlights(value: Highlights): Search


I'm thinking about highlighted(highlights: Highlights). What do you think? Also, add a new line before.

Maybe to use highlight to have meaning as a verb. (I want this search and highlight this)

Can you add a new line before?

drmarjanovic · 2023-03-31T12:50:25Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+import zio.json.ast.Json
+import zio.json.ast.Json.Obj
+
+case class Highlights(fields: Chunk[HighlightField], globalConfig: HighlightConfig = Map.empty) { self =>


Let's add final here.

drmarjanovic · 2023-03-31T12:50:41Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+import zio.json.ast.Json
+import zio.json.ast.Json.Obj
+
+case class Highlights(fields: Chunk[HighlightField], globalConfig: HighlightConfig = Map.empty) { self =>


Can we use config here instead of globalConfig?

We have config parameter in method, maybe it would not be clear.

drmarjanovic · 2023-03-31T12:51:43Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+  def withGlobalConfig(configFieldName: String, config: Json): Highlights =
+    self.copy(globalConfig = self.globalConfig.updated(configFieldName, config))
+
+  def withHighlight(fieldName: String, fieldConfig: HighlightConfig = Map.empty): Highlights =


Do we need Field here also? And maybe use field instead of fieldName.
Alos, I would use config instead of fieldConfig.

drmarjanovic · 2023-03-31T12:52:01Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+case class Highlights(fields: Chunk[HighlightField], globalConfig: HighlightConfig = Map.empty) { self =>
+  def toJson: Json = Obj("highlight" -> Obj(configList: _*).merge(fieldsList))
+
+  def withGlobalConfig(configFieldName: String, config: Json): Highlights =


I would use field here as well.

drmarjanovic · 2023-03-31T12:53:01Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+  type HighlightConfig = Map[String, Json]
+}
+
+case class HighlightField(fieldName: String, fieldConfig: HighlightConfig = Map.empty) {


Can we use field and config?
Also, we are missing final.

arnoldlacko · 2023-04-03T05:56:16Z

modules/library/src/main/scala/zio/elasticsearch/executor/HttpExecutor.scala

+      case (Some(s), Some(h)) => r.query.toJson merge s merge h
+      case (Some(s), _)       => r.query.toJson merge s
+      case (_, Some(h))       => r.query.toJson merge h
+      case _                  => r.query.toJson


I would use explicit None (when _ only stands for None or (None, None)), so that cases could be easily reordered without changing the meaning.

modules/library/src/main/scala/zio/elasticsearch/ElasticHighlight.scala

drmarjanovic · 2023-04-03T14:14:27Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

@@ -311,16 +319,29 @@ object ElasticRequest {
      with HasRouting[SearchRequest]
      with WithSort[SearchRequest] {
    def aggregate(aggregation: ElasticAggregation): SearchAndAggregateRequest
+    def highlights(value: Highlights): Search


Can you add a new line before?

drmarjanovic · 2023-04-03T14:16:06Z

modules/library/src/main/scala/zio/elasticsearch/executor/HttpExecutor.scala

@@ -360,7 +364,10 @@ private[elasticsearch] final class HttpExecutor private (esConfig: ElasticConfig
        case HttpOk =>
          response.body.fold(
            e => ZIO.fail(new ElasticException(s"Exception occurred: ${e.getMessage}")),


Move => ZIO.fail to new line as well.

drmarjanovic · 2023-04-03T14:16:16Z

modules/library/src/main/scala/zio/elasticsearch/executor/HttpExecutor.scala

@@ -463,7 +470,15 @@ private[elasticsearch] final class HttpExecutor private (esConfig: ElasticConfig
        case HttpOk =>
          response.body.fold(
            e => ZIO.fail(new ElasticException(s"Exception occurred: ${e.getMessage}")),


drmarjanovic · 2023-04-03T14:17:09Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+}
+
+final case class HighlightField(field: String, config: HighlightConfig = Map.empty) {
+  def toStringJsonPair: (String, Obj) = field -> Obj(config.toList: _*)


Is this visible to the user?

drmarjanovic · 2023-04-03T14:17:43Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+
+  def withExplicitFieldOrder: Highlights = self.copy(explicitFieldOrder = true)
+
+  private def configList: List[(String, Json)] = config.toList


Please add a new line between. Can we use lazy val instead?

drmarjanovic · 2023-04-03T14:19:07Z

modules/library/src/main/scala/zio/elasticsearch/result/Item.scala

  def documentAs[A](implicit schema: Schema[A]): Either[DecodeError, A] = JsonDecoder.decode(schema, raw.toString)
+
+  lazy val highlights: Option[Map[String, Chunk[String]]] = highlight.flatMap { json =>
+    json.toString.fromJson[Map[String, Chunk[String]]] match {


Can you use .toOption only?

drmarjanovic · 2023-04-03T14:19:55Z

modules/library/src/main/scala/zio/elasticsearch/result/ElasticResult.scala

@@ -63,6 +63,8 @@ final class SearchResult private[elasticsearch] (private val hits: List[Item]) e
        DecodingException(s"Could not parse all documents successfully: ${errors.map(_.message).mkString(",")})")
      }
    }
+
+  def items: ZIO[Any, Nothing, List[Item]] = ZIO.succeed(hits)


We should use alias for ZIO[Any, Nothing, A].

drmarjanovic · 2023-04-03T14:20:31Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+  config: HighlightConfig = Map.empty,
+  explicitFieldOrder: Boolean = false
+) { self =>
+  def toJson: Json = Obj("highlight" -> Obj(configList: _*).merge(fieldsList))


I would use private[elasticsearch] here at least.

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

drmarjanovic · 2023-04-03T15:07:42Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+  config: HighlightConfig = Map.empty,
+  explicitFieldOrder: Boolean = false
+) { self =>
+  def withExplicitFieldOrder: Highlights = self.copy(explicitFieldOrder = true)


Can you move this to the new line?

modules/library/src/main/scala/zio/elasticsearch/result/Item.scala

dbulaja98 · 2023-04-04T10:14:57Z

modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala

@@ -549,6 +553,158 @@ object HttpExecutorSpec extends IntegrationSpec {
            Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
          )
        ) @@ shrinks(0),
+        suite("searching for documents with highlights")(
+          test("successfully find document and return highlight") {


Maybe return document and highlight/with highlight?

dbulaja98 · 2023-04-04T10:16:39Z

modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala

+            Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
+            Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
+          ),
+          test("successfully find document and return highlight using field accessor") {


dbulaja98 · 2023-04-04T10:22:34Z

modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala

+            Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
+            Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
+          ),
+          test("successfully find document and return highlights map") {


dbulaja98 · 2023-04-04T10:26:34Z

modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala

+            Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
+            Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
+          ),
+          test("successfully find document and use global config to return highlight") {


Maybe return document and highlight using global config?

dbulaja98 · 2023-04-04T10:27:06Z

modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala

+            Executor.execute(ElasticRequest.createIndex(firstSearchIndex)),
+            Executor.execute(ElasticRequest.deleteIndex(firstSearchIndex)).orDie
+          ),
+          test("successfully find document and use local config to overwrite global config and return highlight") {


dbulaja98 · 2023-04-04T10:28:34Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+ * limitations under the License.
+ */
+
+package zio.elasticsearch.highlighting


Why not just highlight?

dbulaja98 · 2023-04-04T10:32:12Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+  def withHighlight(field: Field[_, _]): Highlights =
+    withHighlight(field.toString, Map.empty)


In Aggregations and Queries order of methods was like this:
method(f: Field[_, _])
method(f: String),
so maybe we could do same here?

dbulaja98 · 2023-04-04T10:33:54Z

modules/library/src/main/scala/zio/elasticsearch/highlighting/Highlights.scala

+
+  private lazy val configList: List[(String, Json)] = config.toList
+
+  private lazy val fieldsList: Obj =


You have configList: List[(String, String)] above, and here fieldsList: Obj.
Maybe we should call this fieldsListJson?

dbulaja98 · 2023-04-04T10:38:41Z

modules/library/src/main/scala/zio/elasticsearch/ElasticRequest.scala

    def toJson: Json = {
-      val baseJson = (self.from, self.size) match {
-        case (Some(from), Some(size)) =>
-          Obj("from" -> from.toJson) merge Obj("size" -> size.toJson) merge self.query.toJson
-        case (Some(from), None) =>
-          Obj("from" -> from.toJson) merge self.query.toJson
-        case (None, Some(size)) =>
-          Obj("size" -> size.toJson) merge self.query.toJson
-        case _ =>
-          self.query.toJson
-      }
+      val fromJson: Json = self.from.map(f => Obj("from" -> f.toJson)).getOrElse(Obj())

-      sortBy match {
-        case sorts if sorts.nonEmpty =>
-          baseJson merge Obj("sort" -> Arr(self.sortBy.toList.map(_.paramsToJson): _*)) merge aggregation.toJson
-        case _ =>
-          baseJson merge aggregation.toJson
-      }
+      val sizeJson: Json = self.size.map(s => Obj("size" -> s.toJson)).getOrElse(Obj())
+
+      val highlightsJson: Json = self.highlights.map(_.toJson).getOrElse(Obj())
+
+      val sortJson: Json =
+        if (self.sortBy.nonEmpty) Obj("sort" -> Arr(self.sortBy.toList.map(_.paramsToJson): _*)) else Obj()
+
+      fromJson merge sizeJson merge highlightsJson merge sortJson merge self.query.toJson
    }
  }


You are missing aggregations here.

dbulaja98 · 2023-04-04T10:40:20Z

modules/library/src/it/scala/zio/elasticsearch/HttpExecutorSpec.scala

+                  _     = println(docs)
+                  _     = println(aggs)


Remove prints

drmarjanovic requested changes Mar 31, 2023

View reviewed changes

arnoldlacko reviewed Apr 3, 2023

View reviewed changes

modules/library/src/main/scala/zio/elasticsearch/ElasticHighlight.scala Outdated Show resolved Hide resolved

arnoldlacko reviewed Apr 3, 2023

View reviewed changes

modules/library/src/main/scala/zio/elasticsearch/ElasticHighlight.scala Outdated Show resolved Hide resolved

markaya force-pushed the dsl-support-highlights branch from b442cbd to 654a79f Compare April 3, 2023 11:42

drmarjanovic requested changes Apr 3, 2023

View reviewed changes

markaya added 10 commits April 4, 2023 11:29

Implement support highlights

096e1bc

Finish up support for highlights

587f040

Fix code remarsk and add explicit field order

5e8149c

Fix failing linter

ba10cb0

Fix code remarks

9253ce0

Fix linter

8edf6ea

Add highlights parsing

4a37d16

Fix code remarks

dcdfa3d

Implement missing methods and add tests

c1e8594

Rebase and add tests

ef89dca

markaya force-pushed the dsl-support-highlights branch from c0a5599 to ef89dca Compare April 4, 2023 10:11

dbulaja98 requested changes Apr 4, 2023

View reviewed changes

Fix error with aggregations

cfb4500

dbulaja98 previously approved these changes Apr 4, 2023

View reviewed changes

Fix failing tests

8211dc8

markaya dismissed dbulaja98’s stale review via 8211dc8 April 4, 2023 12:05

Fix linter

5eca040

drmarjanovic approved these changes Apr 4, 2023

View reviewed changes

drmarjanovic merged commit 3d2998c into main Apr 4, 2023

drmarjanovic deleted the dsl-support-highlights branch April 4, 2023 12:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(dsl): Support highlights #141

(dsl): Support highlights #141

markaya commented Mar 31, 2023

drmarjanovic Mar 31, 2023

drmarjanovic Mar 31, 2023

drmarjanovic Mar 31, 2023

drmarjanovic Mar 31, 2023

markaya Apr 2, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Mar 31, 2023

drmarjanovic Mar 31, 2023

markaya Apr 2, 2023

drmarjanovic Mar 31, 2023

drmarjanovic Mar 31, 2023

drmarjanovic Mar 31, 2023

arnoldlacko Apr 3, 2023 •

edited

Loading

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

drmarjanovic Apr 3, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023

dbulaja98 Apr 4, 2023


		def withExplicitFieldOrder: Highlights = self.copy(explicitFieldOrder = true)

		private def configList: List[(String, Json)] = config.toList

		def withHighlight(field: Field[_, _]): Highlights =
		withHighlight(field.toString, Map.empty)


		private lazy val configList: List[(String, Json)] = config.toList

		private lazy val fieldsList: Obj =

(dsl): Support highlights #141

(dsl): Support highlights #141

Conversation

markaya commented Mar 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnoldlacko Apr 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arnoldlacko Apr 3, 2023 •

edited

Loading