Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

sql/index/pilosa: return all index ids in lookups #586

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

erizocosmico
Copy link
Contributor

Fixes #585

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
@erizocosmico erizocosmico requested a review from a team January 15, 2019 16:32
@@ -63,6 +64,7 @@ type (
keys []interface{}
expressions []string
operations []*lookupOperation
indexes []string
Copy link
Contributor

@kuba-- kuba-- Jan 15, 2019

Choose a reason for hiding this comment

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

Maybe it should be map[string]int, where key is an index name and value will be order id.
So we'll have auto-uniques (because of map), and then we can sort map entries by value, sth. like:

m := make(map[string]int)
m[indexName] = len(m)
//...
indexes := make([]string, len(m))
for k, v := range m {
    index[v] = k
}
//...
return indexes

WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why the int? We can just use a map[string]struct{} and then sort them.

Also, even if indexes is a map, it may not be unique, because id can be inside that map.

Copy link
Contributor

Choose a reason for hiding this comment

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

int just because we can store in what order we "got" indexes, so at the end you can just re-write it into the slice in the values order (instead of sorting, first).

Copy link
Contributor

Choose a reason for hiding this comment

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

The question is how do we want to show indexes (sorted in the order how filter was parsed, or sorted by name)?
On the other hand, these are "small" numbers so most likely keep the most readable version.

Copy link
Contributor Author

@erizocosmico erizocosmico Jan 16, 2019

Choose a reason for hiding this comment

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

I will use the struct{} only because the int requires some explanation at first sight and the other doesn't. Regarding the order, I think it does not matter much, so we can just sort.

The thing is: with a map we start requiring constructors and so on and before you could just &indexLookup{id: whatever}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so let's keep it what we have so far. I suppose, there is no many cases where we'll use the same index more than once, so maybe map can be kind of overhead.

Copy link
Contributor

@ajnavarro ajnavarro left a comment

Choose a reason for hiding this comment

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

LGTM after @kuba-- comments

@erizocosmico
Copy link
Contributor Author

Done, though I think it unnecessarily complicates the code for very little gain.

@kuba--
Copy link
Contributor

kuba-- commented Jan 16, 2019

Signed-off-by: Miguel Molina <miguel@erizocosmi.co>
@ajnavarro ajnavarro merged commit fa02b97 into src-d:master Jan 16, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants