RediSearch / redisearch-go

Go client for RediSearch
https://redisearch.io
BSD 3-Clause "New" or "Revised" License
298 stars 66 forks source link

MAXTEXTFIELDS support seemingly removed #76

Closed joshuaray closed 4 years ago

joshuaray commented 4 years ago

Not sure if this was intentional or I'm missing where this feature is located now, but it looks like the ability to specify the MAXTEXTFIELDS option when creating a schema has been removed. It was added in PR #28, but looks like a lot of that code has since been re-factored and should have been moved when the index manipulation was moved to schema.go.

filipecosta90 commented 4 years ago

hi there @joshuaray, the ability to specify the MAXTEXTFIELDS option was never merged as it had an issue on the implementation and we decided to split PRs for the different issues it tried to solve. The PR fixed the issue around FT.ALTER support issue #25 and not the MAXTEXTFIELDSflag. This was my comment requesting changes on Dec 5, 2019:

@@ -68,6 +71,9 @@ func NewClient(addr, name string) *Client {
// CreateIndex configues the index and creates it on redis
func (i *Client) CreateIndex(s *Schema) error {
    args := redis.Args{i.name}

    args = append(args, "MAXTEXTFIELDS", fmt.Sprintf("%d", s.Options.MaxTextFields))

@Felixls doing this will cause an error. MAXTEXTFIELDS should not be proceeded by the number of max text fields. It is only a flag. Quoting docs:

MAXTEXTFIELDS: For efficiency, RediSearch encodes indexes differently if they are created with less than 32 text fields. This option forces RediSearch to encode indexes as if there were more than 32 text fields, which allows you to add additional fields (beyond 32) using FT.ALTER.

lets remove it :)

So, this raises two things:

Sorry for the PR name being completely misleading ( I should have renamed it when we decided to push it forward )

joshuaray commented 4 years ago

Got it. It seems like this would really be more adding support for flags in general, not just MAXTEXTFIELDS. So yeah, what you're saying about the PR makes sense.

filipecosta90 commented 4 years ago

Got it. It seems like this would really be more adding support for flags in general, not just MAXTEXTFIELDS. So yeah, what you're saying about the PR makes sense.

Yes. Do you have specific ones that you're missing for your use case? We will push forward supporting all flags/features but we can adjust the priority by our users needs :)

joshuaray commented 4 years ago

MAXTEXTFIELDS is the only one that's holding me up at the moment.

filipecosta90 commented 4 years ago

Hi there @joshuaray , #83 should fix it. Will keep this issue up to date on changes to it.