Auto complete with meta tags #1438
Conversation
replay
force-pushed
the
auto_complete_with_meta_tags
branch
from
4d1c473
to
90d56c2
Compare
the from argument has never been documented and is not used by graphite, so it should be safe to assume that nobody is using it. keeping it would make the queries quite a lot more complicated once we have to also take meta tags into account, so we remove it.
changes the order of the function definitions in various types, so corresponding functions are in the same order everywhere. this only touches the functions used for the routes - /tags - /tags/:tag - /tags/autoComplete/tags - /tags/autoComplete/values
replay
force-pushed
the
auto_complete_with_meta_tags
branch
from
90d56c2
to
3ca07e8
Compare
|
|
||
| i := 0 | ||
| for _, v := range byPath { | ||
| results[i] = *v |
There was a problem hiding this comment.
Why do we go through the trouble of dereferencing/copying them here? Should we just return []*idx.Node instead? At this point everything in byPath has already been cloned.
Or, could we just change byPath to map[string]idx.Node?
There was a problem hiding this comment.
I think you're right that returning []*idx.Node would be an improvement. The reason why it is this way now is just for consistency with .Find(). Note that in some cases like in api/ccache.go:74 the results of .Find() and .FindByTag() are even getting merged together, so i think it's nicer to not have to deal with different types.
I don't think returning map[string]idx.Node makes sense, because that's just unnecessarily keeping redundant information, because the NameWithTags is already a property of the node.
Eventually we'd have to convert it into a slice anyway when preparing the response to the user, unless we also want to change the response format to a map as well, but that would result in unnecessary duplicate data sent over the network.
So I think the best for now is to keep that as it is, to keep everything consistent, and because this PR shouldn't also change Find() to make it consistent with FindByTags() because that's out of scope of the PR. But in a separate PR i'd be happy to change the return type of Find() and FindByTags() to []*idx.Node.
There was a problem hiding this comment.
I like the idea of changing it in the future.
I didn't mean that we should return map[string]idx.Node, I just meant we should use it internally in that method instead of map[string]*idx.Node. We are creating a map of pointers and then immediately dereferencing it when we add it to the slice.
If we decide later to return a []*idx.Node then changing it now doesn't make sense either I suppose.
There was a problem hiding this comment.
if internally we'd use map[string]idx.Node as a temporary struct, before we turn it into a []*idx.Node to return as result, wouldn't that just result in more copying?
You can't reference a map key/value IIRC, so the map values would have to be copied one more time when building the []*idx.Node
There was a problem hiding this comment.
Right, I'm talking about the current state of things. We are not returning []*idx.Node, so why are we creating a map of pointers and then just converting them straight away?
There was a problem hiding this comment.
you're right, as long as we return []idx.Node we could also build a map[string]idx.Node instead of map[string]*idx.Node.
There was a problem hiding this comment.
I'm not sure if there's really any advantage to doing that though. changing it to map[string]idx.Node safes us one dereference and one pointer which is only in memory for milliseconds, on the other hand, using map[string]idx.Node requires us to do more copying.
This implements the auxiliary API endpoints for tag operations. These include:
/tags/autoComplete/tags)/tags/autoComplete/values)/tags)/tags/:tag([0-9a-zA-Z]+))This PR is based on the branch of #1433, so it makes no sense to review it before that one is merged