Scripting API (eval, evalSha, scriptExists, scriptLoad) #269

anatolysergeev · 2021-01-08T13:15:23Z

Issue #158

That is a try to implement scripting API for redis.
This PR have debatable changes: removing sealed state for traits Input and Output. I couldn't find an answer for questions "why they are should be sealed?", "why we don't want to give the opportunity for customers of this library to implement their own encoders and decoders".
In test I've written an example how that could be easy to implement own decoder for customer if traits wouldn't be sealed.
Thanks, I'm waiting for approval to continue coding in this approach or for comments how better it should be implemented.

delete sealed mark from Input and Output traits

CLAassistant · 2021-01-08T13:15:27Z

All committers have signed the CLA.

mijicd · 2021-01-08T13:17:15Z

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

@@ -8,8 +8,8 @@ import scala.util.matching.Regex
 import zio.Chunk
 import zio.duration.Duration

-sealed trait Input[-A] {
-  private[redis] def encode(data: A): Chunk[RespValue.BulkString]
+trait Input[-A] {


Please leave it sealed. Input and output aren't really data encoders, but "shapes" of command inputs and outputs, hence no need for flexibility there. There's an issue for making data encoding more flexible (see #214).

Ok, add redisencoder, redisdecoder according to #214

mijicd · 2021-01-08T13:18:24Z

redis/src/main/scala/zio/redis/Output.scala

@@ -3,8 +3,7 @@ package zio.redis
 import zio.Chunk
 import zio.duration._

-sealed trait Output[+A] {
-
+trait Output[+A] {


Please leave it sealed.

mijicd · 2021-01-08T13:18:59Z

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

+import zio.ZIO
+import zio.redis.Input.EvalInput
+import zio.redis._
+import Scripting._


Please move this to trait.

mijicd · 2021-01-08T13:19:40Z

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

+
+private[redis] object Scripting {
+
+  final def evalCommand[K: Input, A: Input, R: Output]: RedisCommand[Script[K, A], R] =


Please rename to eval. This will change as soon as you adjust input.

mijicd · 2021-01-08T13:20:31Z

redis/src/main/scala/zio/redis/options/Scripting.scala

+
+trait Scripting {
+
+  case class Script[K, A](lua: String, keys: Seq[K], args: Seq[A])


Let's not use Chunk instead of Seq. Also, make it sealed. More important, this isn't really an option.

Sorry, i don't understand that comment((

Sorry, my english broke, I wanted to say let's use Chunk instead of Seq and let's avoid using Seq.

mijicd · 2021-01-08T13:21:23Z

redis/src/test/scala/zio/redis/ScriptingSpec.scala

+        },
+        testM("return custom data type") {
+
+          case class CustomData(count: Long, avg: Long, pair: (Int, String))


Please make it final.

mijicd · 2021-01-08T13:30:30Z

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

    def encode(data: Iterable[A]): Chunk[RespValue.BulkString] =
      data.foldLeft(Chunk.empty: Chunk[RespValue.BulkString])((acc, a) => acc ++ input.encode(a))
  }

+  case class EvalInput[K, A](keysInput: Input[K], argsInput: Input[A]) extends Input[Script[K, A]] {


I don't think this is correct. What you have in Script should be specified as an input, i.e.:

string script

varargs keys

varargs args

Also, be consistent and make it final.

Hopefully, I understood you right and did how you wanted)

return sealed state for traits input, output add test for errors

mijicd · 2021-01-08T23:31:27Z

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

    def encode(data: Iterable[A]): Chunk[RespValue.BulkString] =
      data.foldLeft(Chunk.empty: Chunk[RespValue.BulkString])((acc, a) => acc ++ input.encode(a))
  }

+  case object EvalInput extends Input[(String, Seq[Chunk[Byte]], Seq[Chunk[Byte]])] {


Please no Seq.

mijicd · 2021-01-08T23:31:50Z

redis/src/main/scala/zio/redis/options/Scripting.scala

+
+trait Scripting {
+
+  sealed case class Script[K, A](lua: String, keys: Seq[K], args: Seq[A]) {


As mentioned in my previous comments, this isn't an option, and it isn't even necessary anymore.

Script has been deleted. IMHO this class showed relation between keys, args and lua script, that's why i wrote it)

mijicd · 2021-01-08T23:34:37Z

redis/src/main/scala/zio/redis/RedisEncoder.scala

+
+import java.nio.charset.StandardCharsets
+
+trait RedisEncoder[A] {


I'd rename to Encoder.

mijicd · 2021-01-08T23:34:47Z

redis/src/main/scala/zio/redis/RedisDecoder.scala

+
+import zio.IO
+
+trait RedisDecoder[A] {


I'd rename to Decoder.

redis/src/main/scala/zio/redis/Output.scala

mijicd · 2021-01-08T23:42:31Z

@quelgar this PR introduces the notion of codecs you mentioned in #214. Now, it doesn't propagates them throughout the API, but we might as well settle upon this API now and propagate in a separate ticket.

One thing I'm not sure is Decoder being aware of RespValue, but it might be the only way to encode it (too late for me to think).

fix linter errors delete script case class replace all seq on chunks

add script debug add script exists add script flush add script kill add script load

mijicd

@anatolysergeev Please run sbt prepare as well.

@quelgar Just a reminder to take a look at codec part :).

mijicd · 2021-01-11T22:41:57Z

redis/src/main/scala/zio/redis/Decoder.scala

+
+import zio.IO
+
+trait Decoder[A] {


I am unsure about this. RespValue should not leak in the client code. If we're allowing custom decoders (and we should), I think they should work on byte chunks.

mijicd · 2021-01-11T22:42:18Z

redis/src/main/scala/zio/redis/Encoder.scala

+
+import zio.Chunk
+
+trait Encoder[A] {


Let's add summoner here.

redis/src/main/scala/zio/redis/Output.scala

mijicd · 2021-01-11T22:46:27Z

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

+   *         You have to write decoder that would convert
+   *         redis protocol value to a suitable type for your app
+   *
+   * @since 2.6.0


I'd remove @since as it refers to Redis version, which doesn't really matches the lib version.

mijicd · 2021-01-11T22:48:51Z

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

+  final val Eval: RedisCommand[(String, Chunk[Chunk[Byte]], Chunk[Chunk[Byte]]), RespValue] =
+    RedisCommand("EVAL", EvalInput, RespValueOutput)
+
+  final val EvalSHA: RedisCommand[(String, Chunk[Chunk[Byte]], Chunk[Chunk[Byte]]), RespValue] =


Please rename to EvalSha.

mijicd · 2021-01-11T22:50:57Z

redis/src/test/scala/zio/redis/ScriptingSpec.scala

+import zio.test._
+
+trait ScriptingSpec extends BaseSpec {
+  val scriptingSpec: Spec[Annotations with RedisExecutor, TestFailure[Any], TestSuccess] =


Lots of tests seems to be ignored, what's the problem?

test for flash command need specific test environment to run, due to tests constancy i wrote ignore.
with other tests i was straggling to wrote them that they would work like i want. For example test for command kill i don;t understand why but don't want to kill the script((, test for debug when you start command yes or sync block everything in redis and don't want to turn off debug mode.
I think that debug, kill should be run on in specific environment, too.

Hm, in that case i propose shipping the testable commands in this PR and the other through follow up pull requests, i.e. once the env is figured out.

quelgar · 2021-01-12T10:58:44Z

@quelgar this PR introduces the notion of codecs you mentioned in #214. Now, it doesn't propagates them throughout the API, but we might as well settle upon this API now and propagate in a separate ticket.

One thing I'm not sure is Decoder being aware of RespValue, but it might be the only way to encode it (too late for me to think).

Yeah, I can't help thinking we're missing an opportunity to simplify here. The fact there's commands here that use the RespValueOutput no-op output, then does the decoding via a different type, seems like code smell. We're bypassing the Output mechanism altogether, and we'll potentially end up re-implementing decoding to common types like Instant and Duration.

Can we pull Output up to the API level as a type class? The unsafe methods are always private. I think user-provided implementations could all be built from the basic Output implementations, via map. We might want a variant of map to support failure.

anatolysergeev · 2021-01-12T17:19:04Z

Can we pull Output up to the API level as a type class? The unsafe methods are always private. I think user-provided implementations could all be built from the basic Output implementations, via map. We might want a variant of map to support failure.

That was my first implementation. But @mijicd proposed to do Encoder. And i rewrote Decoder too. Main points were consistency with Input and save ADT for Output. I agree that for contributors to this library it would harder to understand why there are 2 "similar" services, but for users i don't think that would be a problem.

I have more worries about that or mb i don't understand something

I am unsure about this. RespValue should not leak in the client code. If we're allowing custom decoders (and we should), I think they should work on byte chunks.

User should implement their own Resp deserialization or how that would work?
The problem is that eval could return any type of RespValue, and only user know what lua script would return

mijicd · 2021-01-12T17:26:09Z

@anatolysergeev Well, the original implementation unsealed the output and exposed it in user-land. I don't think we should do it, and I recalled an issue that proposed encoder / decoder mechanism. If we're aiming to simplify, then let's propose combinators to the existing input / output as @quelgar wrote.

anatolysergeev · 2021-01-12T18:06:41Z

Ok. I want to do some conclusion here, what i am going to do

delete Encoder/Decoder
Add map combinator to Input (Output already has that one). i want to remind u, that It would look like this:
sealed is stayed for Input and Output
add some catch method to Input and Output
RespValueOutput is stayed too because we need initial output of respvalue

am i right?) It would be nice to have approve from both of you. Thanks.

P.S. hard decision actually i don't even know, what of this i like more)) Can i ask why for input and output sealed status so important?

mijicd · 2021-01-12T18:14:01Z

@anatolysergeev i believe i answered that one. Originally, input and output where envisioned as command shapes. Since there's a finite amount of those, it makes sense to seal them. However, now we're effectively giving them more power. I'd say try to keep them sealed, if we need to unseal, we can do that.

quelgar · 2021-01-13T11:43:25Z

@anatolysergeev sounds reasonable to me

I think for Input we want contramap instead of map:

sealed trait Input[A] {

  self =>

  final def contramap[B](f: B => A): Input[B] = new Input {
    def encode(data: B) = self.encode(f(b))
  }
}

On the point "eval can return any RespValue", I was wondering if for a particular script you'd know what type of value to expect back. But I guess it's all dynamically typed, so your script could return a bulk string on one code path and an integer on another. What kind of monster Scala programmer would write such a script? But we should support it regardless 🤣

add contramap to Input delete decoder/encoder

anatolysergeev · 2021-01-14T17:57:17Z

I haven't done methods that would be catch failures(
I have been thinking how to write the methods and best what i can figured out is this:

final def catchSome[E <: RedisError, B >: A](rePF: PartialFunction[E, B]): Output[B] = {
 val thPF: PartialFunction[Throwable, B] = { th => throw th }
  new Output[B] {
    protected def tryDecode(respValue: RespValue): B = Try(self.tryDecode(respValue)).recover(rePF.orElse[Throwable, B](thPF)).get
  }
}

mijicd · 2021-01-22T21:09:23Z

@anatolysergeev I am sorry for my way overdue response. My workload took its toll. After a discussion with John, I realized that zio-schema is close to release. That said, I don't think we should invent the new codec "machinery" but used the one provided by zio-schema instead. With that in mind, I propose the following course of actions:

extract all encoding and decoding from this PR (stay in string domain) in a way that it provides a part of scripting commands implementable with the current design
introduce the codecs from zio-schema in the follow-up
propagate encoding changes throughout the library (this would involve changing the type constraints on our API level and postponing codec operations in the interpreter)

anatolysergeev · 2021-02-08T16:26:51Z

Hello @mijicd . I am sorry for my long response too and my inactivity. I've read your comment and don't see the solution with string domain for "eval" appropriate and don't want to do it just to close the issue.
Has zio-schema been released? I saw some release tags there.
Could I have a try to write a solution with schema instead current encoders/decoders (input/output) approach? Or we want to remove Input/Output totally from this project in favor of zio.Schema and there are no way to do something for eval now?

mijicd · 2021-02-08T16:33:10Z

@anatolysergeev Please don't remove input / output. As originally discussed, they're intended to specify shapes of commands. However, we need arbitrary input / output, that could be polymorphic. In the user land (API functions), this parameter would be type parameterized to require a schema. So yes, I propose digging into zio-schema API (it's been released), and drafting the solution. I do have a sketch of it, so if you're willing to wait until the weekend, we could have a pairing session. WDYT?

anatolysergeev · 2021-02-08T18:16:13Z

I think that a great idea and i'm very excited about that. But if you mean in "pair session" an online conversation i should warn you that i'm not confident in my English speaking skills, all in all i would try my best))

# Conflicts: # redis/src/main/scala/zio/redis/Input.scala # redis/src/main/scala/zio/redis/Output.scala # redis/src/main/scala/zio/redis/RedisCommand.scala # redis/src/main/scala/zio/redis/api/Hashes.scala # redis/src/main/scala/zio/redis/api/Keys.scala # redis/src/main/scala/zio/redis/api/Lists.scala # redis/src/main/scala/zio/redis/api/Sets.scala # redis/src/main/scala/zio/redis/api/SortedSets.scala # redis/src/main/scala/zio/redis/api/Streams.scala # redis/src/main/scala/zio/redis/package.scala # redis/src/test/scala/zio/redis/ApiSpec.scala # redis/src/test/scala/zio/redis/OutputSpec.scala

# Conflicts: # redis/src/main/scala/zio/redis/api/Hashes.scala # redis/src/main/scala/zio/redis/api/Keys.scala # redis/src/main/scala/zio/redis/api/Sets.scala # redis/src/main/scala/zio/redis/api/SortedSets.scala # redis/src/main/scala/zio/redis/api/Streams.scala

add result builder support for mGet method clean some unnecessary explicit types in tests

# Conflicts: # redis/src/main/scala/zio/redis/ResultBuilder.scala

mijicd · 2022-01-06T08:12:30Z

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

+      val encodedKeys       = keys.foldLeft[Chunk[BulkString]](Chunk.empty)((cur, next) => cur ++ inputK.encode(next))
+      val encodedArgs       = args.foldLeft[Chunk[BulkString]](Chunk.empty)((cur, next) => cur ++ inputV.encode(next))


Shouldn't

Suggested change

val encodedKeys = keys.foldLeft[Chunk[BulkString]](Chunk.empty)((cur, next) => cur ++ inputK.encode(next))

val encodedArgs = args.foldLeft[Chunk[BulkString]](Chunk.empty)((cur, next) => cur ++ inputV.encode(next))

val encodedKeys = keys.flatMap(inputK.encode)

val encodedArgs = args.flatMap(inputV.encode)

mijicd · 2022-01-06T08:13:32Z

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

+  trait Implicits {
+    implicit val bytesEncoder: Input[Chunk[Byte]] = ByteInput
+    implicit val booleanInput: Input[Boolean]     = BoolInput
+    implicit val stringInput: Input[String]       = StringInput
+    implicit val longInput: Input[Long]           = LongInput
+  }
+
+  object Implicits extends Implicits


This seems to be used only in tests. If that's the case, please define them there instead.

mijicd · 2022-01-06T08:15:03Z

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

+    args: Chunk[A]
+  ): ResultOutputBuilder = new ResultOutputBuilder {
+    def returning[R: Output]: ZIO[RedisExecutor, RedisError, R] = {
+      val command = RedisCommand(Eval, EvalInput(implicitly[Input[K]], implicitly[Input[A]]), implicitly[Output[R]])


If possible, add summoners for Input and Output. It'll make the call site cleaner.

mijicd · 2022-01-06T08:21:05Z

redis/src/test/scala/zio/redis/ScriptingSpec.scala

+
+  final case class CustomData(count: Long, avg: Long, pair: (Int, String))
+
+  object CustomData {


I would not nest items not directly related to CustomData in its companion object.

mijicd · 2022-01-06T08:21:28Z

redis/src/test/scala/zio/redis/ScriptingSpec.scala

+    private val tryDecodeLong: RespValue => Long = {
+      case RespValue.Integer(value) => value
+      case other                    => throw ProtocolError(s"$other isn't a integer type")
+    }
+    private val tryDecodeString: RespValue => String = {
+      case s @ RespValue.BulkString(_) => s.asString
+      case other                       => throw ProtocolError(s"$other isn't a string type")
+    }


You already have these in LongOutput and StringOutput. Please check the rest implicits you defined here.

Yes, that's true. But Outputs required codecs and we don't have schema codecs implicit in the Output.map. I had a long thought about that, i can not figure out how better to handle this.

Codecs are serialize, deserialize that we put into cache ignoring redis protocol, but here we work with redis protocol

It seems not very good to have initialization of codec in few place (now RedisExacutor has dependency of it)

It looks like we should do something with .map method because inside it we actually has required implicit of codec, but how to get it from there 🤔

One of idea was to add new .mapWithCodec - not sure about a name))
But i don't like that solution, it's looks like a crunch to me 🤔

final def mapWithCodec[B](f: Codec => A => B): Output[B] = new Output[B] { protected def tryDecode(respValue: RespValue)(implicit codec: Codec): B = f(codec)(self.tryDecode(respValue)) } implicit val decoder: Output[CustomData] = RespValueOutput.mapWithCodec { implicit codec => { case RespValue.Array(elements) => val count = LongOutput.unsafeDecode(elements(0)) val avg = LongOutput.unsafeDecode(elements(1)) val pair = elements(2) match { case RespValue.Array(elements) => (LongOutput.unsafeDecode(elements(0)).toInt, StringOutput.unsafeDecode(elements(1))) case other => throw ProtocolError(s"$other isn't an array type") } CustomData(count, avg, pair) case other => throw ProtocolError(s"$other isn't an array type") } }

add eval command

5e52717

delete sealed mark from Input and Output traits

anatolysergeev requested a review from a team as a code owner January 8, 2021 13:15

mijicd requested changes Jan 8, 2021

View reviewed changes

add redis encoder/decoder

3ff7cf1

return sealed state for traits input, output add test for errors

mijicd reviewed Jan 8, 2021

View reviewed changes

anatolysergeev added 2 commits January 9, 2021 03:02

rename decoder and encoder

baef574

fix linter errors delete script case class replace all seq on chunks

add evalsha

ab9fe63

add script debug add script exists add script flush add script kill add script load

anatolysergeev changed the title ~~WIP scripting API~~ Scripting API Jan 10, 2021

fix linter

4bf4832

mijicd reviewed Jan 11, 2021

View reviewed changes

anatolysergeev added 3 commits January 14, 2021 20:38

use input/output instead decoder/encoder

29081c5

add contramap to Input delete decoder/encoder

delete since

c369df4

delete scripting commands without tests

b68e9d5

return private[redis] for encode method

5493dfb

anatolysergeev changed the title ~~Scripting API~~ Scripting API (eval, evalSha, scriptExists, scriptLoad) Jan 14, 2021

Drop override

51a2ef3

mijicd and others added 17 commits January 2, 2022 00:16

Propagate through streams API

8a989b6

Fix benchmark compilation issues

c966679

Improve type inference

418ef78

Tackle nested contexts in sorted sets

1467786

Tackle nested contexts in hashes

5c4d0e5

Tackle nested contexts in keys

325484a

Fix formatting

7e8818e

Fix benchmark error

22944ec

Fix compilation issues

7abbb73

Fix compilation issues

81da33c

Fix test expectations

f3c9a0f

Try to help with [Improve polymorphic output handling zio#404] (#1)

7595188

change output type for incrByFloat method to Double (#2)

8904e9e

add result builder support for mGet method clean some unnecessary explicit types in tests

Merge branch 'feature/result-builder' into scripting

19e0259

add output result builder for scripting methods

d57cb55

anatolysergeev marked this pull request as draft January 3, 2022 13:47

anatolysergeev added 2 commits January 5, 2022 01:37

Merge branch 'master' into scripting

ed4dcd2

# Conflicts: # redis/src/main/scala/zio/redis/ResultBuilder.scala

fix linter issues

a6cdaa0

anatolysergeev marked this pull request as ready for review January 4, 2022 22:48

anatolysergeev requested a review from mijicd January 4, 2022 22:49

anovakovic01 previously approved these changes Jan 5, 2022

View reviewed changes

mijicd requested changes Jan 6, 2022

View reviewed changes

fix review

e37a4f5

anatolysergeev dismissed anovakovic01’s stale review via e37a4f5 January 6, 2022 12:02

mijicd approved these changes Jan 6, 2022

View reviewed changes

mijicd merged commit e49e320 into zio:master Jan 6, 2022

mijicd mentioned this pull request Jan 6, 2022

Implement scripting API #158

Closed

7 tasks

anatolysergeev mentioned this pull request Nov 12, 2022

Implement remaining script API commands #656

Closed


		private[redis] object Scripting {

		final def evalCommand[K: Input, A: Input, R: Output]: RedisCommand[Script[K, A], R] =


		trait Scripting {

		case class Script[K, A](lua: String, keys: Seq[K], args: Seq[A])


		trait Scripting {

		sealed case class Script[K, A](lua: String, keys: Seq[K], args: Seq[A]) {


		import java.nio.charset.StandardCharsets

		trait RedisEncoder[A] {

		val encodedKeys = keys.foldLeft[Chunk[BulkString]](Chunk.empty)((cur, next) => cur ++ inputK.encode(next))
		val encodedArgs = args.foldLeft[Chunk[BulkString]](Chunk.empty)((cur, next) => cur ++ inputV.encode(next))


		final case class CustomData(count: Long, avg: Long, pair: (Int, String))

		object CustomData {

Scripting API (eval, evalSha, scriptExists, scriptLoad) #269

Scripting API (eval, evalSha, scriptExists, scriptLoad) #269

Conversation

anatolysergeev commented Jan 8, 2021 • edited Loading

CLAassistant commented Jan 8, 2021 • edited Loading

mijicd Jan 8, 2021 • 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

mijicd commented Jan 8, 2021

mijicd left a 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

anatolysergeev Jan 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

quelgar commented Jan 12, 2021

anatolysergeev commented Jan 12, 2021 • edited Loading

mijicd commented Jan 12, 2021

anatolysergeev commented Jan 12, 2021 • edited Loading

mijicd commented Jan 12, 2021

quelgar commented Jan 13, 2021

anatolysergeev commented Jan 14, 2021

mijicd commented Jan 22, 2021

anatolysergeev commented Feb 8, 2021 • edited Loading

mijicd commented Feb 8, 2021

anatolysergeev commented Feb 8, 2021 • 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

anatolysergeev Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

anatolysergeev Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

anatolysergeev commented Jan 8, 2021 •

edited

Loading

CLAassistant commented Jan 8, 2021 •

edited

Loading

mijicd Jan 8, 2021 •

edited

Loading

anatolysergeev Jan 12, 2021 •

edited

Loading

anatolysergeev commented Jan 12, 2021 •

edited

Loading

anatolysergeev commented Jan 12, 2021 •

edited

Loading

anatolysergeev commented Feb 8, 2021 •

edited

Loading

anatolysergeev commented Feb 8, 2021 •

edited

Loading

anatolysergeev Jan 6, 2022 •

edited

Loading

anatolysergeev Jan 6, 2022 •

edited

Loading