fix(browse): Fix browse pagination and multi-browse path issue #2984
Conversation
|
|
||
| return new BrowseResult().setMetadata(browseResultMetadata) | ||
| .setEntities(new BrowseResultEntityArray(browseResultEntityList)) | ||
| .setNumEntities(numGroups + numEntities) |
There was a problem hiding this comment.
should this be numElements or total or some other name that indicates it is a sum of entities + groups?
I would expect numEntities to be numEntities + group.map(group -> group.count).sum()
There was a problem hiding this comment.
Yeah the naming here is confusing... I'd agree I'd think num entities would be either
a. The number of concrete entities matching the exact path OR
b. The total number of entities somewhere under this path
In fact, it may be useful to include both of these in the response separately.
Can we nip this in the bud with this PR?
There was a problem hiding this comment.
Unfortunately BrowseResult PDL is in datahub-gms right now. We can make the above changes after moving them over here.
| final SearchResponse entitiesResponse = | ||
| client.search(constructEntitiesSearchRequest(indexName, path, requestMap, from, size), | ||
| client.search(constructEntitiesSearchRequest(indexName, path, requestMap, entityFrom, entitySize), | ||
| RequestOptions.DEFAULT); |
There was a problem hiding this comment.
small optimization available here- if entitySize is 0 we don't have to do this query
There was a problem hiding this comment.
or do we need to for counts?
There was a problem hiding this comment.
Yeah. I was going to do that until you mentioned we need the whole counts
| @@ -210,4 +202,17 @@ | |||
| return null; | |||
| }); | |||
| } | |||
|
|
|||
| @Action(name = "getNumEntities") | |||
There was a problem hiding this comment.
I don't want to disrupt this PR too much - but what do you think about naming this getTotalEntityCount? getNumEntities is a bit too vague IMO
| * | ||
| * @param entityName name of the entity | ||
| */ | ||
| long docCount(@Nonnull String entityName); |
There was a problem hiding this comment.
How does this hold up given Timeseries aspects? Can we call this getSnapshotDocCount or something of the type to illustrate this divergence?
There was a problem hiding this comment.
Timeseries service doesn't implement SearchService. This is specific to search backend (which is only used by versioned aspects)
| .type("BrowseResults", typeWiring -> typeWiring | ||
| .dataFetcher("entities", new AuthenticatedResolver<>( | ||
| new EntityTypeBatchResolver( | ||
| ENTITY_TYPES.stream().collect(Collectors.toList()), | ||
| (env) -> ((BrowseResults) env.getSource()).getEntities())) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
side note- we should really put these two in a configureSearchAndBrowseResolvers...
Fixed browse pagination bug, where pagination just doesn't work.
Also fixed the multi-browse path issue where a group entry is shown when it is actually an entity.
Also added two end points, getNumEntities and batchGetNumEntities to efficiently get number of entities per entity type.
Checklist