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

how to add new index to old ones #632

Open
hamidrabedi opened this issue Sep 8, 2024 · 11 comments · Fixed by #645
Open

how to add new index to old ones #632

hamidrabedi opened this issue Sep 8, 2024 · 11 comments · Fixed by #645
Labels
enhancement New feature or request feature good first issue Good for newcomers help wanted Extra attention is needed

Comments

@hamidrabedi
Copy link

So i have these indexes on the first startup:

	if err := r.Repository.CreateIndex(context.Background(), func(schema om.FtCreateSchema) rueidis.Completed {
		return schema.
			FieldName("$.user.owner").As("owner").Tag().
			FieldName("$.user.name").As("name").Tag().
			Build()
	}); err != nil {
		logs.Warning(err)
	}

now I need to add a new index:

	if err := r.Repository.CreateIndex(context.Background(), func(schema om.FtCreateSchema) rueidis.Completed {
		return schema.
			FieldName("$.user.owner").As("owner").Tag().
			FieldName("$.user.name").As("name").Tag().
			FieldName("$.user.id").As("id").Tag().
			Build()
	}); err != nil {
		logs.Warning(err)
	}

But I get an index already exists error. I need my Redis to be persistent and can't flush everything.

@rueian
Copy link
Collaborator

rueian commented Sep 11, 2024

Hi @hamidrabedi,

You need to use FT.ALTER on your own currently, just like what CreateIndex does under the hood.

return r.client.Do(ctx, cmdFn(r.client.B().FtCreate().Index(r.idx).OnJson().Prefix(1).Prefix(r.prefix+":").Schema())).Error()

We may also add an AlterIndex method to the repository interface.

@rueian rueian added enhancement New feature or request good first issue Good for newcomers feature labels Sep 11, 2024
@hamidrabedi
Copy link
Author

Yes that would be a nice feature to have, writing raw code or removing and recreating indexes doesn't seem to be a good solution.

@rueian rueian added the help wanted Extra attention is needed label Sep 25, 2024
@imvtsl
Copy link
Contributor

imvtsl commented Oct 5, 2024

Hey @rueian
Can I have a go at this issue?

Just to clarify few things before I begin working on the fix:
1- We want to add AlterIndex method to this repository.
2- This method would perform the same functionality as FT.ALTER.

Is this a correct understanding of the requirement?

@rueian
Copy link
Collaborator

rueian commented Oct 5, 2024

Hey @rueian Can I have a go at this issue?

Just to clarify few things before I begin working on the fix: 1- We want to add AlterIndex method to this repository. 2- This method would perform the same functionality as FT.ALTER.

Is this a correct understanding of the requirement?

Sure, thanks for your interest in this.

  1. Add AlterIndex to the type Repository[T any] interface and implement it in both type JSONRepository[T any] struct and type HashRepository[T any] struct. The implementations should be similar to the existing CreateIndex.
  2. Yes, the implementations should use the FT.ALTER command by r.client.B().FtAlter().
  3. Come up with test cases to show that the implementations work in the hash_test.go and json_test.go files.

@imvtsl
Copy link
Contributor

imvtsl commented Oct 5, 2024

Thanks for the quick response. I will begin working on this.

@imvtsl
Copy link
Contributor

imvtsl commented Oct 12, 2024

Hi @rueian
Just wanted to give an update. I created above PR and verified that it is working as expected by checking manually using redis-cli. I am working on test cases for the change.
Thanks!

@hamidrabedi
Copy link
Author

@imvtsl will there be automatic update if index fields were changed ? that would be nice

@rueian
Copy link
Collaborator

rueian commented Oct 13, 2024

Hi @hamidrabedi, do you have a feeling about what an automatic update API should look like?

@hamidrabedi
Copy link
Author

@rueian, suppose you have a new Redis instance and you call the create index function. If the index has not been created before, it will be created.

Obviously, most of the time there will be updates to your projects, and you might need to remove or add indexes. So instead of using CreateIndex, I want to use something like CreateOrUpdateIndex. This function checks the existing indexes in Redis and ensures that my indexes are present; if not, it adds them.

Regarding deleting an index, I have observed that many migration libraries do not handle deletion for various reasons, so users should handle deleting indexes themselves. However, we could have an option for it—like an argument in the CreateOrUpdateIndex function that, if provided by the user, would indicate that index deletion should happen as well.

This is just a basic usage that users can encounter, right now for this problem all I can do is to call flushall on initialization of my project, or handling the index Updates by connecting to the redis. I can't think of any other usecases at the moment, but I think basic auto update will do the trick for now.

@rueian
Copy link
Collaborator

rueian commented Oct 15, 2024

This looks like it requires a set of high-level APIs, probably including our own indexes DSL.

But I am thinking of another approach, how about always creating a new index with FT.CREATE and transparently redirecting application queries to it by using FT. ALIASADD? I think that will be much easier.

@hamidrabedi
Copy link
Author

Oh yeah much easier, or like dropping and recreating the index, but I think alias is better and you have lower downtime because the old index won't be affected. can delete the old index afterwards.

@rueian rueian reopened this Oct 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants