Support SORT command in Keys API #302

sloanesturz · 2021-03-04T04:27:33Z

Fixes #218.

SORT has two different response types:

SORT myKey # -> returns the sorted values in the list stored at myKey
SORT myKey STORE resultKey # -> returns the number of elements sorted, writes values to resultKey

This is not easily expressible in Scala, so I created two separate functions, one for SORT ... STORE ... and one for SORT ...

This is my first commit to zio-redis, please let me know if I am not following any important functional or stylistic guidelines!

CLAassistant · 2021-03-04T04:27:37Z

All committers have signed the CLA.

mijicd

@sloanesturz thank you for taking care of this! I left some general remarks, but more specifically, I don't think we should expose two separate commands. I would propose to:

apply the proposed fixes
represent missing terms (e.g. alpha)
unify commands into single sort

Please take a look at geo API, as there's quite a few options defined there, and it might serve as an "inspiration" here :).

mijicd · 2021-03-05T22:11:20Z

redis/src/main/scala/zio/redis/api/Keys.scala

+  final def sort(
+    key: String,
+    by: Option[String] = None,
+    limitOffset: Option[(Int, Int)] = None,


We have this defined among sorted sets (see here). I propose that you reuse it and move the option itself to shared options.

mijicd · 2021-03-05T22:11:32Z

redis/src/main/scala/zio/redis/api/Keys.scala

+    key: String,
+    by: Option[String] = None,
+    limitOffset: Option[(Int, Int)] = None,
+    desc: Boolean = false,


Order is defined as geo options here. Please do the same approach:

move to shared

use it instead of plain boolean

sloanesturz · 2021-03-05T22:30:27Z

OK, thanks for the pointers to those options fields -- definitely more clean to use those instead of my approach. The pattern in the Geo package has a lot of overlap and is clearly the right way to go.

unify commands into single sort

What should the return type of this method be? If we SORT ... STORE ..., Redis will return a number. If we SORT ..., Redis will return a list of values. To unify these two I could always return a Chunk[String] and if you had happened to SORT ... STORE ... you just "have to know" that the Chunk[String] will have one entry, the number of values. To me, that seems pretty kludgy but I suppose it does sorta replicate what Redis is doing.

mijicd · 2021-03-05T22:34:31Z

One option might be to define a custom response type (ADT of possible responses), but let's play around with this, perhaps something better pops up.

sloanesturz · 2021-03-05T22:38:45Z

OK, let me take a crack at your first suggestions before taking on the second one. In my experience, returning an Either[Long, Seq[String]] that is fully is defined by the kinds of arguments to the function is probably not what we want here because you end up writing code like

redis.sort(key, storeAt = None).map {
  case _: Right => ??? // as a developer, I know this can't happen, annoying the compiler makes me handle this
  case _: Left =>  doMyImplementation() // the actual implementation I want to do
}

redis/src/main/scala/zio/redis/Input.scala

mijicd · 2021-03-08T08:54:27Z

redis/src/main/scala/zio/redis/api/Keys.scala

+  def sortArguments(
+    by: Option[String],
+    limitOffset: Option[(Int, Int)],
+    desc: Boolean,
+    get: List[String],
+    alpha: Boolean
+  ): List[String] =
+    by.map(sortBy => List("BY", sortBy)).getOrElse(List.empty) ++
+      limitOffset.map { case (count, offset) => List("LIMIT", offset.toString, count.toString) }
+        .getOrElse(List.empty) ++
+      get.flatMap(getAt => List("GET", getAt)) ++
+      List(if (desc) "DESC" else "ASC") ++
+      (if (alpha) List("ALPHA") else List.empty)
+


Should this be deleted?

mijicd

Almost there! Since we opted for this approach of command per output (with a good reason, as you elaborated), do you mind doing a quick scan of the similar commands and filing them in an issue so that we can align the API? 🙏

mijicd · 2021-03-09T01:12:49Z

redis/src/main/scala/zio/redis/Input.scala

@@ -126,6 +138,11 @@ object Input {
      Chunk(encodeString("LIMIT"), encodeString(data.offset.toString), encodeString(data.count.toString))
  }

+  final case class ListInput[-A](input: Input[A]) extends Input[List[A]] {
+    override private[redis] def encode(data: List[A]): Chunk[RespValue.BulkString] =


Suggested change

override private[redis] def encode(data: List[A]): Chunk[RespValue.BulkString] =

def encode(data: List[A]): Chunk[RespValue.BulkString] =

However, see the comment below, this might be unnecessary.

mijicd · 2021-03-09T01:16:52Z

redis/src/main/scala/zio/redis/api/Keys.scala

+        StringInput,
+        OptionalInput(ByInput),
+        OptionalInput(LimitInput),
+        ListInput(GetInput),


This can actually be changed to OptionalInput(NonEmptyList[GetInput]).

sloanesturz · 2021-03-09T06:19:19Z

Filed #306 for the GEO functions.

mijicd

Nice work! 🚀

Sloane Sturzenegger added 4 commits March 3, 2021 20:00

Checkpoint

87393f8

it works

b971d09

scalafmt

ccf554f

scalafmt, docs

dc5a96d

sloanesturz requested a review from a team as a code owner March 4, 2021 04:27

test:scalafmt, oops

9eaa65a

mijicd reviewed Mar 5, 2021

View reviewed changes

Sloane Sturzenegger added 2 commits March 5, 2021 15:43

Follow the pattern of how to format optional parameters

7dc7da4

unit tests

781a012

sloanesturz commented Mar 5, 2021

View reviewed changes

redis/src/main/scala/zio/redis/Input.scala Outdated Show resolved Hide resolved

Sloane Sturzenegger added 2 commits March 5, 2021 15:54

fix docs

ffc88ec

Use more straightforward methods on Chunk

b51c16e

mijicd reviewed Mar 8, 2021

View reviewed changes

delete old code

e775386

mijicd reviewed Mar 9, 2021

View reviewed changes

update to use the better get argument

d303062

mijicd approved these changes Mar 9, 2021

View reviewed changes

mijicd merged commit a390a8b into zio:master Mar 9, 2021

sloanesturz mentioned this pull request Mar 9, 2021

Support STORE in Georadius commands #308

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support SORT command in Keys API #302

Support SORT command in Keys API #302

sloanesturz commented Mar 4, 2021

CLAassistant commented Mar 4, 2021 •

edited

Loading

mijicd left a comment

mijicd Mar 5, 2021

mijicd Mar 5, 2021

sloanesturz commented Mar 5, 2021

mijicd commented Mar 5, 2021

sloanesturz commented Mar 5, 2021

mijicd Mar 8, 2021

sloanesturz Mar 9, 2021

mijicd left a comment

mijicd Mar 9, 2021

mijicd Mar 9, 2021

sloanesturz Mar 9, 2021

sloanesturz commented Mar 9, 2021

mijicd left a comment

	override private[redis] def encode(data: List[A]): Chunk[RespValue.BulkString] =
	def encode(data: List[A]): Chunk[RespValue.BulkString] =

Support SORT command in Keys API #302

Support SORT command in Keys API #302

Conversation

sloanesturz commented Mar 4, 2021

CLAassistant commented Mar 4, 2021 • edited Loading

mijicd left a comment

Choose a reason for hiding this comment

mijicd Mar 5, 2021

Choose a reason for hiding this comment

mijicd Mar 5, 2021

Choose a reason for hiding this comment

sloanesturz commented Mar 5, 2021

mijicd commented Mar 5, 2021

sloanesturz commented Mar 5, 2021

mijicd Mar 8, 2021

Choose a reason for hiding this comment

sloanesturz Mar 9, 2021

Choose a reason for hiding this comment

mijicd left a comment

Choose a reason for hiding this comment

mijicd Mar 9, 2021

Choose a reason for hiding this comment

mijicd Mar 9, 2021

Choose a reason for hiding this comment

sloanesturz Mar 9, 2021

Choose a reason for hiding this comment

sloanesturz commented Mar 9, 2021

mijicd left a comment

Choose a reason for hiding this comment

CLAassistant commented Mar 4, 2021 •

edited

Loading