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

#342: Implement hrandfield command and support it in TestExecutor #370

Merged
merged 3 commits into from
May 11, 2021

Conversation

jgoday
Copy link
Contributor

@jgoday jgoday commented May 7, 2021

Closes #342

Implements HRANDFIELD as two different methods.
As mentioned in the redis documentation, HRANGEFIELD can return a field or an array of fields/values, depending on the value of the count parameter.
This implementation has two different methods, differentiated by their return value.
If count is negative, it allows to return repeated field/values.

* hRandField[K: Schema, V: Schema](key: K): ZIO[RedisExecutor, RedisError, Option[V]]
* hRandField[K: Schema, V: Schema](
    key: K,
    count: Long,
    withValues: Boolean = false
  ): ZIO[RedisExecutor, RedisError, Chunk[V]]

@jgoday jgoday requested a review from a team as a code owner May 7, 2021 19:16
@jgoday jgoday mentioned this pull request May 7, 2021
3 tasks
}

/**
* Return an array of distinct fields. The array's length is either `count` or the hash's number of fields if `count` is greater.
Copy link
Collaborator

@jxnu-liguobin jxnu-liguobin May 8, 2021

Choose a reason for hiding this comment

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

Please keep the document consistent with other commands.Such as initials, full stop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jxnu-liguobin Sorry for that. I have tried to solve this issues, let me know if it's correct now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. @param key of a sorted set
  2. @param key Key of a sorted set

There are still two styles of documentation, but I'm not sure which is the standard(Maybe the second one? Scala source code uses the second one.). An additional PR may be needed to unify all document comments.

Copy link
Member

Choose a reason for hiding this comment

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

Definitely the second one. However, if there are no proboems with the implementation, let's merge this and do the docs unification in the follow up.

for {
kvs <- keysAndValues
fields <- selectValues(count.get, kvs.toVector)
fieldsAndValues = fields.map { case (k, v) => Seq(k, v) }.flatten
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of flatten it should be possible to directly use flatMap

} else if (count.isDefined) {
for {
kvs <- keysAndValues
keys = kvs.map(_._1)
Copy link
Contributor

Choose a reason for hiding this comment

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

For better readability, I prefer kvs.map {case (k, _) => k} but up to you just a suggestion.

@@ -972,6 +972,49 @@ private[redis] final class TestExecutor private (
} yield RespValue.Array(Chunk.fromIterable(values))
)

case api.Hashes.HRandField =>
val key = input(0).asString
val count = if (input.size == 1) None else input(1).asString.toLongOption
Copy link
Contributor

Choose a reason for hiding this comment

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

I think .toLongOption is not available in scala 2.12

@hcwilhelm
Copy link
Contributor

LGTM

@mijicd mijicd merged commit 02ad6ae into zio:master May 11, 2021
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.

Missing Command HRANDFIELD
4 participants