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

Feat rueidiscompat redisearch #676

Merged
merged 49 commits into from
Nov 25, 2024

Conversation

unknowntpo
Copy link
Contributor

@unknowntpo unknowntpo commented Nov 20, 2024

Changes and cautions:

  • docker-compose
    • add compat-redisearch service for testing RediSearch with RESP2
  • FT.CREATE:
    • search_command.json
      • Add options: INDEXEMPTY, INDEXMISSING, WITHCOUNT, ADDSCORES
      • Add field_type: GEOSHAPE
  • FT.AGGREGATE:
    • search_command.json:
      • Add option: WITHCOUNT
  • FT.SEARCH
    • go-redis doesn't implement SUMMARIZE option
    • Add option: WITHCOUNT

rueidiscompat/pipeline.go Outdated Show resolved Hide resolved
@unknowntpo unknowntpo marked this pull request as ready for review November 24, 2024 11:13
message.go Outdated
Comment on lines 475 to 482
func (r RedisResult) IsMap() bool {
return r.val.IsMap()
}

// IsArray delegates to RedisMessage.IsArray
func (r RedisResult) IsArray() bool {
return r.val.IsArray()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
func (r RedisResult) IsMap() bool {
return r.val.IsMap()
}
// IsArray delegates to RedisMessage.IsArray
func (r RedisResult) IsArray() bool {
return r.val.IsArray()
}
func (r RedisResult) isMap() bool {
return r.val.IsMap()
}
// IsArray delegates to RedisMessage.IsArray
func (r RedisResult) isArray() bool {
return r.val.IsArray()
}

Can we make these 2 methods private first? I am concerned about making these direct delegations to r.val as public APIs. I think we should at least ask users to check the r.err first.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if we make IsMap private, functions in command.go can not access it,
I think we could check if any error exists, then delegate to r.val.IsMap.

func (r RedisResult) IsMap() bool {
	if r.err != nil {
		panic(fmt.Errorf("RedisResult has error: %w, please check the error before calling RedisResult.IsMap", r.err))
	}
	return r.val.IsMap()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UPDATE: I'll use res.ToMessage() to get underlying message first, then do the IsMap, IsArray check.

Comment on lines 37 to 47
func Sleep(ctx context.Context, dur time.Duration) error {
t := time.NewTimer(dur)
defer t.Stop()

select {
case <-t.C:
return nil
case <-ctx.Done():
return ctx.Err()
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we remove unused functions in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay.

message.go Outdated
@@ -456,7 +456,7 @@ func (r RedisResult) ToMap() (v map[string]RedisMessage, err error) {
if r.err != nil {
err = r.err
} else {
v, err = r.val.ToMap()
v, err = r.val.AsMap()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need this change? I think we already have an AsMap delegation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is accidentally committed, I've reverted it.

@rueian rueian merged commit 131aad3 into redis:main Nov 25, 2024
30 checks passed
@unknowntpo unknowntpo deleted the feat-rueidiscompat-redisearch branch November 25, 2024 13:24
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.

2 participants