Implement FUNCTION group of commands

[elena-kolevska]

Implemented load, delete and flush. Working on function list

[elena-kolevska]

Solves #2400, #2397 and #2395

[monkey92t]

Good, please continue...

[elena-kolevska]

@vmihailenco , @monkey92t, if the approach I had with FUNCTION LIST is ok with you, I'll continue implementing the WITHCODE variant of it and all the rest of the FUNCTIONS group.

[monkey92t]

redis.RdsFunction looks weird 😁

[elena-kolevska]

redis.RdsFunction looks weird 😁

Indeed. I was reluctant to name it just Function though, cause even though it's not a reserved word in Go, when you're reading it in code, Redis Functions would probably not be the first thing to come to mind.
If you think that's not a concern though, I'm totally open to changing it.

[monkey92t]

redis.RdsFunction looks weird 😁

Indeed. I was reluctant to name it just Function though, cause even though it's not a reserved word in Go, when you're reading it in code, Redis Functions would probably not be the first thing to come to mind. If you think that's not a concern though, I'm totally open to changing it.

redis.Function{} looks fine since go-redis only works with redis, right? redis.RdsFunction looks weird as pool.PoolName.

[elena-kolevska]

Ok, fixed. Thanks for the insight.

[vmihailenco]

Looks good so far 👍

[elena-kolevska]

Looking at the FUNCTION LIST command arguments now and I would very much appreciate your opinion on the API design. I had a few ideas:

Option 1

redis.FunctionsList()			// []Library{}
redis.FunctionsListWithCode()		// []Library{}

redis.FunctionList("mylib")		// Library{}
redis.FunctionListWithCode("mylib")	// Library{}

Option 2

redis.FunctionListAll()			// []Library{}
redis.FunctionListAllWithCode()		// []Library{}

redis.FunctionList("mylib")		// Library{}
redis.FunctionListWithCode("mylib")	// Library{}

Option 3

redis.Libraries()			// []Library{}
redis.LibrariesWithCode()		// []Library{}

redis.Library("mylib")			// Library{}
redis.LibraryWithCode("mylib")		// Library{}

Option 4

redis.ListLibraries()			// []Library{}
redis.ListLibrariesWithCode()		// []Library{}

redis.ListLibrary("mylib")		// Library{}
redis.ListLibraryWithCode("mylib")	// Library{}

Option 5

q := FunctionListQuery{
	LibraryName: "mylib",
	WithCode: true
}
redis.FunctionList(q)				// []Library{}

Options 1 and 2

• Simple, descriptive, matches Redis API very closely.
• Can become too messy if new arguments are added to FUNCTION LIST in the future.
• Confusing because it actually returns libraries, not functions, but that's a problem in the server's API, not ours. The question is: do we want to "improve" some Redis api inconsistencies in the client? It could have both pros and cons.

Options 3 and 4

• Intuitive, readable, matches the reality of what is returned but doesn't match the Redis API.
• Can become too messy if new arguments are added to FUNCTION LIST in the future.

Option 5

• I like the idea of a Query object cause it keeps our API simple
• Allows for easy implementation of any eventual future Redis API changes in the FUNCTION LIST command
• I don't like returning a single Library object wrapped in an array when q.LibraryName is not null

[monkey92t]

I think:

func FunctionList(libraryNamePattern string, withCode bool) []Library

We should try to keep consistent with redis commands, and should not increase the cost for users to learn go-redis.

[elena-kolevska]

Yeah, I definitely see the point there; I just feel that it wouldn't look very pretty when we call the method without the optional arguments:

redis.FunctionList("", false)

The following would actually be equivalent, and it looks better, but then we would have to teach people to use it that way; it's not obvious:

redis.FunctionList("*", false)

At the very minimum, I could make the library name pattern its own struct and pass nil. Still not great, but I think it looks a bit better than an empty string. What do you think?

redis.FunctionList(nil, false)

// Or with an argument:
redis.FunctionList(LibraryNamePattern{"mylib"}, false)

[vmihailenco]

I think it should be either

func FunctionList(libraryNamePattern string, withCode bool) []Library

or Option 5

rdb.FunctionList(FunctionList{
	LibraryName: "mylib",
	WithCode: true,
})				// []Library{}

The latter is somewhat more readable.

I don't like returning a single Library object wrapped in an array when q.LibraryName is not null

We could add a helper function to retrieve a single library:

rdb.FunctionList(...).Result() // []Library
rdb.FunctionList(...).First() // *Library

[vmihailenco]

Also check:

// SORT list LIMIT 0 2 ASC
vals, err := rdb.Sort(ctx, "list", &redis.Sort{Offset: 0, Count: 2, Order: "ASC"}).Result()

// ZRANGEBYSCORE zset -inf +inf WITHSCORES LIMIT 0 2
vals, err := rdb.ZRangeByScoreWithScores(ctx, "zset", &redis.ZRangeBy{
    Min: "-inf",
    Max: "+inf",
    Offset: 0,
    Count: 2,
}).Result()

// ZINTERSTORE out 2 zset1 zset2 WEIGHTS 2 3 AGGREGATE SUM
vals, err := rdb.ZInterStore(ctx, "out", &redis.ZStore{
    Keys: []string{"zset1", "zset2"},
    Weights: []int64{2, 3}
}).Result()

[monkey92t]

I've considered "Option 5" too, if you all like it then we'll use it. 😄

[elena-kolevska]

Great that we have an agreement :)

I implemented everything as you suggested, with the only difference being that the .First() method uses value instead of pointer semantics. I'm relatively new to Go, and I'm totally open to opinions here; I was just following the guideline that if it's a struct type and you're not expecting it to be mutated, you should use value semantics so you're not polluting the heap. As I said, though: new to Go, totally open to learning! ;)

I also implemented a FirstVal() method for completeness.

[monkey92t]

Library is large enough that we can use ptr. What is the point of adding FirstVal()?

[elena-kolevska]

Ok, I'll change it to a pointer then.

Re: FirstVal(), my reasoning was to have the matching "single-library" method for Val() as we have for Result(). I'm not a fan of the name, though, I gotta admit.

rdb.FunctionList(...).Result() // []Library, err
rdb.FunctionList(...).First() // *Library, err

rdb.FunctionList(...).Val() // []Library
rdb.FunctionList(...).FirstVal() // *Library

[monkey92t]

@elena-kolevska I tidied up the code.....I hope you won't mind. 😊

[elena-kolevska]

Of course not! Quite the contrary, it’s great for my learning. 🙏

[monkey92t]

@elena-kolevska Do you want to add more commands? such: function kill, function status, or do we add it later?

[elena-kolevska]

Since I'll only be able to come back to this next week, I'd suggest we close this PR now, and I'll add what's missing next week. There was also a PR on FUNCTION KILL already; we should check if we can use it.

[monkey92t]

OK, we don't need to close it, this PR can already do some work, we will add function kill, function status later.

[monkey92t]

@elena-kolevska Waiting for you to finish the rest of the work. 😄

[elena-kolevska]

OK, we don't need to close it, this PR can already do some work, we will add function kill, function status later.

Yeah, sorry, I meant "merge it"

I'm very much looking forward to it too ;)
Thank you all for your help with this PR.

[vmihailenco]

@elena-kolevska thanks for your contribution and putting an extra effort into API design! It's clean and simple 👍