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

add new command: FILTERVALS #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

manosriram
Copy link

No description provided.

@@ -1,6 +1,7 @@
package core

import (
"path/filepath"
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this a heavy library for a basic usecase? A lightweight implementation of the same is possible via reflect and regexp. Note: reflect would be needed to deduce the type " INT, FLOAT, DOUBLE, BOOLEAN, STRING", so regex could match the values better. This however could be a non-issue for the time-being.

core/store.go Outdated
value.LastAccessedAt = getCurrentClock()
}

return matchedValues
Copy link
Contributor

@rohanverma94 rohanverma94 Oct 25, 2022

Choose a reason for hiding this comment

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

Also can't we convert the [][2]string to the string here itself? To encode for a very specific type with a specialized encoder implementation seems tedious for any new command in the future.
It would be easy to process the matched values cumulatively and return string here itself.

0) 0) user:1
    1) value:1
1) 0) user:2
    1) value:2

Copy link
Author

Choose a reason for hiding this comment

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

by string, you mean []string? because we'll have a list of matches right?

Copy link
Contributor

Choose a reason for hiding this comment

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

@manosriram the return value should be an array to two-tuple allowing we to pipe it with different commands (in the future)

Copy link
Author

@manosriram manosriram Oct 28, 2022

Choose a reason for hiding this comment

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

Copy link
Contributor

@rohanverma94 rohanverma94 Oct 29, 2022

Choose a reason for hiding this comment

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

@manosriram Since the requirement is to return an array of two-tuple so that it can be piped to another set of commands. So something on these lines maybe-

type filtervalTuple struct {
    Rkey string; 
    Rvalue interface{}
}

func FilterVals(pattern string) []filtervalTuple {
    rdevice := regexp.MustCompile(pattern)
    var result []filtervalTuple

    for key, value := range store {
        val := value.(string)
        ok := rdevice.MatchString(val)

        if ok {
            result = append(result, filtervalTuple{Rkey:key, Rvalue:val})
        }
    }

    return result
}

Check a sample code here! - https://rohanverma.godbolt.org/z/oeY4rab4s

@arpitbbhayani any further input if you can give then it would be helpful for @manosriram

Copy link
Author

Choose a reason for hiding this comment

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

@rohanverma94 not sure if the regex will match wildcards, updated other parts except that.

Copy link
Contributor

Choose a reason for hiding this comment

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

@manosriram It's just a sample code for simplifying the concept 😃

Copy link
Author

Choose a reason for hiding this comment

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

@rohanverma94 okay 😅

func FilterVals(pattern string) []string {
var matchedValues []string
for key, value := range store {
val := value.Value.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we not use inherent encoding that we are maintaining instead of reflect?

Copy link
Contributor

Choose a reason for hiding this comment

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

@manosriram this would not work. Use type encodings to check. Not this. Each Dice object holds the type and encoding information. .(string) is an expensive operation. we maintain type encoding information.

clean up print statement

modify [][2] string to []string: filtervals

return filtered key-value pairs as a list

added new case in Encode and struct for filterval tuple

remove func in resp.go

rename var
@rohanverma94
Copy link
Contributor

@manosriram Ask arpit for review. He knows the requirements well.

@manosriram manosriram requested review from arpitbbhayani and removed request for rohanverma94 October 29, 2022 13:30
func FilterVals(pattern string) []string {
var matchedValues []string
for key, value := range store {
val := value.Value.(string)
Copy link
Contributor

Choose a reason for hiding this comment

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

@manosriram this would not work. Use type encodings to check. Not this. Each Dice object holds the type and encoding information. .(string) is an expensive operation. we maintain type encoding information.

Comment on lines +79 to +82
ok, err := filepath.Match(pattern, val)
if err != nil {
return []filterValTuple{}, err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

we are only doing wildcard match. this is an expensive operation again. please write a simple wildcard matcher and then we will extend the solution if needed.

Comment on lines +13 to +17
type filterValTuple struct {
Rkey string
Rvalue interface{}
}

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 creating a filterVal type, create a generic KV tuple type because we will need it for other commands as well.

Copy link
Contributor

@rohanverma94 rohanverma94 Oct 30, 2022

Choose a reason for hiding this comment

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

@arpitbbhayani The project is based on Go 1.17, and we don't have generic types in that yet. We can still enable this via https://twitter.com/go_perf/status/1427540843175555086?s=20&t=3vX8nwQV5E4aye66nh2G0w . This is nonperformant.

Or you can bump the project version to Go 1.18.

@yashs360
Copy link
Contributor

@manosriram Thanks for the contribution. There are conflicts atm.
The code base has moved forward lot. Please rebase and re-submit if you are still keen to fix this. We will be cleaning up old PR's if they are inactive.

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.

None yet

4 participants