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 support for LCS Command #2480

Merged
merged 18 commits into from
Mar 24, 2023
Merged

Conversation

SoulPancake
Copy link
Contributor

@SoulPancake SoulPancake commented Mar 10, 2023

Fixes #2403

command.go Outdated Show resolved Hide resolved
@monkey92t
Copy link
Collaborator

monkey92t commented Mar 10, 2023

my thoughts:

type LCSMatch struct {
	Matches []LCSMatchedPosition
	Len     int
}

type LCSMatchedPosition struct {
        // or A/B ?
	Key1     LCSPosition
	Key2     LCSPosition

        // only for withMatchLen is true
	MatchLen int
}

type LCSPosition struct {
	Start int
	End   int
}

// lcs key1 key2
func LCS(key1, key2 string) *StringCmd {}

// lcs key1 key2 len
func LCSLen(key1, key2 string) *IntCmd {}

// lcs key1 key2 idx minmatchlen minMatchLen withmatchlen
// or *LCSMatchCmd
func LCSIdx(key1, key2 string, minMatchLen int, withMatchLen bool) *LCSCmd {}

//--------------

// OR
type LCSQuery struct {
	Key1         string
	Key2         string
	Len          bool
	Idx          bool
	MinMatchLen  int
	WithMatchLen bool
}

func LCS(key1, key2 string, len, idx bool, minMatchLen int, withMatchLen bool) *LCSCmd {}
func LCS(q *LCSQuery) *LCSCmd{}

😀😀😀😀 comments are welcome~

@monkey92t
Copy link
Collaborator

exp:

type LCSMatch struct {
	MatchString string
	Matches     []LCSMatchedPosition
	Len         int64
}

type LCSMatchedPosition struct {
	// or A/B ?
	Key1 LCSPosition
	Key2 LCSPosition

	// only for withMatchLen is true
	MatchLen int
}

type LCSPosition struct {
	Start int
	End   int
}

type LCSCmd struct {
	baseCmd

	// 1: match string
	// 2: match len
	// 3: match idx LCSMatch
	readType uint8
	val      *LCSMatch
}

func (cmd *LCSCmd) readReply(rd *proto.Reader) (err error) {
	lcs := &LCSMatch{}
	switch cmd.readType {
	case 1:
		// match string
		lcs.MatchString, err = rd.ReadString()
	case 2:
		// match len
		if lcs.Len, err = rd.ReadInt(); err != nil {
			return err
		}
	case 3:
		// read LCSMatch
		if err = rd.ReadFixedMapLen(2); err != nil {
			return err
		}

		key, err := rd.ReadString()
		if err != nil {
			return err
		}

		// we should not depend on the order of the map.
		switch key {
		case "matches":
			n, err := rd.ReadArrayLen()
			if err != nil && err != Nil {
				return err
			}
			for i := 0; i < n; i++ {
				// read LCSMatchedPosition
				// miss..
			}
		case "len":
			if lcs.Len, err = rd.ReadInt(); err != nil {
				return err
			}
		}
	}

	cmd.val = lcs
	return nil
}

type LCSQuery struct {
	Key1         string
	Key2         string
	Len          bool
	Idx          bool
	MinMatchLen  int
	WithMatchLen bool
}

func LCS(ctx context.Context, q *LCSQuery) *LCSCmd {
	// ...miss
	return &LCSCmd{}
}

// call
lcs, err := rdb.LCS(ctx, &LCSQuery{
	//...
}).Result()

lcs.MatchString()
lcs.Len()
lcs.Matches[0].Key1.Start

@monkey92t
Copy link
Collaborator

any thoughts on my example?

@SoulPancake
Copy link
Contributor Author

SoulPancake commented Mar 11, 2023

any thoughts on my example?

Thanks for your example!
It seems like a good approach
@monkey92t I am trying to build using your example only as you can see in the latest pushes

@SoulPancake
Copy link
Contributor Author

Hi @monkey92t
Need some help here
I am not sure if I should implement the MarshalBinary() function for the interface slice or should I read some other way
I am having difficulty in this, Can you please guide me

@monkey92t
Copy link
Collaborator

I don't quite understand what you mean. What does this have to do with MarshalBinary?

@SoulPancake
Copy link
Contributor Author

SoulPancake commented Mar 22, 2023

@monkey92t As I'm getting the error for the same on the test
And it is mentioned https://github.com/SoulPancake/go-redis/blob/e69c70c7b6e63da86eb5853ddd493bea7dda2a1d/commands.go#L1403 here to implement it
Is this the right approach?

@monkey92t
Copy link
Collaborator

No, that's not it. I will make some modifications to the PR later.

@monkey92t monkey92t marked this pull request as ready for review March 22, 2023 16:31
@SoulPancake
Copy link
Contributor Author

Thanks @monkey92t :) I was overcomplicating it

@SoulPancake
Copy link
Contributor Author

@monkey92t As this looks good , we can merge it right?
Is there anything else left to do here?

Copy link
Contributor Author

@SoulPancake SoulPancake left a comment

Choose a reason for hiding this comment

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

LGTM

commands_test.go Show resolved Hide resolved
commands_test.go Outdated Show resolved Hide resolved
commands_test.go Outdated Show resolved Hide resolved
@SoulPancake
Copy link
Contributor Author

@monkey92t The build failure seems unrelated

@monkey92t
Copy link
Collaborator

@monkey92t The build failure seems unrelated

We use redis-server/sentinel/cluster to perform tests, and sometimes encounter failures due to network errors.

@SoulPancake
Copy link
Contributor Author

@monkey92t Had a general question, how were you able to run the build actions again without pushing a new commit?

@monkey92t
Copy link
Collaborator

@monkey92t Had a general question, how were you able to run the build actions again without pushing a new commit?

You need to manually build or rebuild the projects that you have permissions for in Github Actions.

@monkey92t monkey92t merged commit 6790337 into redis:master Mar 24, 2023
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.

Add support for LCS command
2 participants