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

I need to drop the index,but not found this func #94

Closed
is-a-gamer opened this issue Dec 2, 2020 · 8 comments · Fixed by #96
Closed

I need to drop the index,but not found this func #94

is-a-gamer opened this issue Dec 2, 2020 · 8 comments · Fixed by #96

Comments

@is-a-gamer
Copy link
Contributor

Sometimes I just need to delete a separate index and should not use it client.Drop()

@filipecosta90 filipecosta90 self-assigned this Dec 2, 2020
@filipecosta90 filipecosta90 removed their assignment Dec 2, 2020
@filipecosta90
Copy link
Collaborator

Hi there @is-a-gamer , agree.
Given that for v1.6 and v2.0 we have FT.DROP and FT.DROPINDEX I would take this opportunity to add:

wdyt @gkorland @MeirShpilraien ?


Apart from it, @is-a-gamer do you want to submit a PR, or do you want us to do it? Happy to review :)

@MeirShpilraien
Copy link
Collaborator

@filipecosta90 why not stick to the same name as in the api, i.e, Drop and DropIndex?

@filipecosta90
Copy link
Collaborator

@filipecosta90 why not stick to the same name as in the api, i.e, Drop and DropIndex?

Drop() is already taken since the beginning of the client. Given we can't overload we need a different name :)

@is-a-gamer
Copy link
Contributor Author

Apart from it, @is-a-gamer do you want to submit a PR, or do you want us to do it? Happy to review :)

No

I added a method to the client.go

func (i *Client) DropIndex(deleteDocument bool) (err error) {
	conn := i.pool.Get()
	defer conn.Close()

	if deleteDocument {
		_, err = conn.Do("FT.DROPINDEX", i.name, "DD")
	} else {
		_, err = conn.Do("FT.DROPINDEX", i.name)
	}
	return
}

Although it can work normally in my process, I think you should do it more correctly

@filipecosta90
Copy link
Collaborator

@filipecosta90 why not stick to the same name as in the api, i.e, Drop and DropIndex?

Drop() is already taken since the beginning of the client. Given we can't overload we need a different name :)

@MeirShpilraien, bumping this one. I believe we should go with due to the reasons discussed above:

@shumin1027
Copy link
Contributor

@filipecosta90
how about

// Deletes the index.
// By default, FT.DROPINDEX does not delete the document hashes associated with the index. Adding the DD option deletes the hashes as well.
func (i *Client) dropIndex(keepdocs bool) error {
	conn := i.pool.Get()
	defer conn.Close()

	var err error = nil
	if keepdocs {
		_, err = conn.Do("FT.DROPINDEX", i.name)
	} else {
		_, err = conn.Do("FT.DROPINDEX", i.name, "DD")
	}
	return err
}

// Deletes the index and deletes the hashes as well
func (i *Client) DropIndex() error {
	return i.dropIndex(false)
}

// Deletes the index but does not delete the document hashes associated with the index
func (i *Client) DropIndexWithKeepdocs() error {
	return i.dropIndex(true)
}

@filipecosta90
Copy link
Collaborator

@filipecosta90
how about

// Deletes the index.
// By default, FT.DROPINDEX does not delete the document hashes associated with the index. Adding the DD option deletes the hashes as well.
func (i *Client) dropIndex(keepdocs bool) error {
	conn := i.pool.Get()
	defer conn.Close()

	var err error = nil
	if keepdocs {
		_, err = conn.Do("FT.DROPINDEX", i.name)
	} else {
		_, err = conn.Do("FT.DROPINDEX", i.name, "DD")
	}
	return err
}

// Deletes the index and deletes the hashes as well
func (i *Client) DropIndex() error {
	return i.dropIndex(false)
}

// Deletes the index but does not delete the document hashes associated with the index
func (i *Client) DropIndexWithKeepdocs() error {
	return i.dropIndex(true)
}

@shumin1027 I would use DropIndex( string indexName, bool deleteDocs ) for one reason. Please notice that DD argument maps directly to deleteDocs argument. On your function signature we have the inverse keepdocs which might confuse users.

@filipecosta90
Copy link
Collaborator

@is-a-gamer @shumin1027 do you guys want to propose a PR. Will gladly review it :)

@filipecosta90 filipecosta90 linked a pull request Dec 18, 2020 that will close this issue
filipecosta90 added a commit that referenced this issue Dec 18, 2020
* [add] added DropIndex method

* Included DropIndex() example. Extended method description

* [add] Included extra test to cover all DropIndex() conditions

Co-authored-by: 何鸿志 <hehongzhi@njxtc.com>
Co-authored-by: filipecosta90 <filipecosta.90@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants