Skip to content
This repository has been archived by the owner on Jun 26, 2024. It is now read-only.

fix(explore): order by on eds attributes #127

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

fix(explore): order by on eds attributes #127

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
0a5e0c5
fix(explore): order by on eds attributes
skjindal93 Apr 4, 2022
e3f5535
change the limit variable
skjindal93 Apr 25, 2022
f9483a2
Merge branch 'main' into fix+explore+eds+order+by
skjindal93 Apr 25, 2022
735f2b4
Merge branch 'main' into fix+explore+eds+order+by
skjindal93 Aug 27, 2023
998534a
spotless
skjindal93 Aug 27, 2023
a5826ce
fix vulnerabilities
skjindal93 Aug 27, 2023
d9c4e9c
Merge branch 'main' into fix+explore+eds+order+by
skjindal93 Oct 13, 2023
b4b2851
better comment
skjindal93 Oct 13, 2023
7c6a52b
fix vulnerability
skjindal93 Oct 13, 2023
4c5f357
push limit and offset to EQS
skjindal93 Oct 30, 2023
9680686
upgrade grpc service framework
skjindal93 Oct 30, 2023
53df6ea
upgrade dependencies
skjindal93 Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions gateway-service-api/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ protobuf {
// the identifier, which can be referred to in the "plugins"
// container of the "generateProtoTasks" closure.
id("grpc_java") {
artifact = "io.grpc:protoc-gen-grpc-java:1.56.0"
artifact = "io.grpc:protoc-gen-grpc-java:1.57.2"
}
}
generateProtoTasks {
Expand All @@ -44,7 +44,7 @@ sourceSets {
}

dependencies {
api(platform("io.grpc:grpc-bom:1.56.0"))
api(platform("io.grpc:grpc-bom:1.57.2"))
api("io.grpc:grpc-protobuf")
api("io.grpc:grpc-stub")
api("javax.annotation:javax.annotation-api:1.3.2")
Expand Down
2 changes: 1 addition & 1 deletion gateway-service-factory/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ plugins {
}

dependencies {
api("org.hypertrace.core.serviceframework:platform-grpc-service-framework:0.1.52")
api("org.hypertrace.core.serviceframework:platform-grpc-service-framework:0.1.59")
skjindal93 marked this conversation as resolved.
Show resolved Hide resolved

implementation(project(":gateway-service-impl"))
}
6 changes: 3 additions & 3 deletions gateway-service-impl/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,8 @@ dependencies {

implementation("org.hypertrace.entity.service:entity-service-client:0.8.56")
implementation("org.hypertrace.entity.service:entity-service-api:0.8.56")
implementation("org.hypertrace.core.grpcutils:grpc-context-utils:0.12.0")
implementation("org.hypertrace.core.grpcutils:grpc-client-utils:0.12.0")
implementation("org.hypertrace.core.grpcutils:grpc-context-utils:0.12.2")
implementation("org.hypertrace.core.grpcutils:grpc-client-utils:0.12.2")
implementation("org.hypertrace.core.serviceframework:platform-metrics:0.1.52")

// Config
Expand All @@ -41,5 +41,5 @@ dependencies {
testImplementation("org.mockito:mockito-core:4.10.0")
testImplementation("org.mockito:mockito-inline:4.10.0")
testImplementation("org.apache.logging.log4j:log4j-slf4j-impl:2.17.1")
testImplementation("io.grpc:grpc-netty:1.56.0")
testImplementation("io.grpc:grpc-netty:1.57.2")
}
4 changes: 1 addition & 3 deletions ...way-service-impl/src/main/java/org/hypertrace/gateway/service/explore/ExploreService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,9 +110,7 @@ private boolean isContextAnEntityType(ExploreRequest request) {

private IRequestHandler getRequestHandler(
ExploreRequest request, Map<String, AttributeMetadata> attributeMetadataMap) {
if (isContextAnEntityType(request)
&& !hasTimeAggregations(request)
&& !request.getGroupByList().isEmpty()) {
if (isContextAnEntityType(request) && !hasTimeAggregations(request)) {
ExpressionContext expressionContext =
new ExpressionContext(
attributeMetadataMap,
Expand Down
2 changes: 1 addition & 1 deletion ...mpl/src/main/java/org/hypertrace/gateway/service/explore/entity/EntityRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public EntityRequestHandler(
@Override
public ExploreResponse.Builder handleRequest(
ExploreRequestContext requestContext, ExploreRequest exploreRequest) {
// Track if we have Group By so we can determine if we need to do Order By, Limit and Offset
// Track if we have Group By, so we can determine if we need to do Order By, Limit and Offset
// ourselves.
if (!exploreRequest.getGroupByList().isEmpty()) {
requestContext.setHasGroupBy(true);
Expand Down
19 changes: 19 additions & 0 deletions ...c/main/java/org/hypertrace/gateway/service/explore/entity/EntityServiceEntityFetcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,15 @@
import org.hypertrace.gateway.service.common.converters.EntityServiceAndGatewayServiceConverter;
import org.hypertrace.gateway.service.common.util.AttributeMetadataUtil;
import org.hypertrace.gateway.service.common.util.ExpressionReader;
import org.hypertrace.gateway.service.common.util.OrderByUtil;
import org.hypertrace.gateway.service.entity.config.EntityIdColumnsConfigs;
import org.hypertrace.gateway.service.explore.ExploreRequestContext;
import org.hypertrace.gateway.service.v1.common.OrderByExpression;
import org.hypertrace.gateway.service.v1.explore.ExploreRequest;

public class EntityServiceEntityFetcher {
private static final int DEFAULT_ENTITY_REQUEST_LIMIT = 10_000;

private final AttributeMetadataProvider attributeMetadataProvider;
private final EntityIdColumnsConfigs entityIdColumnsConfigs;
private final EntityQueryServiceClient entityQueryServiceClient;
Expand Down Expand Up @@ -57,7 +60,18 @@ private EntityQueryRequest buildRequest(

addGroupBys(exploreRequest, builder);
addSelections(requestContext, exploreRequest, builder);

builder.setLimit(DEFAULT_ENTITY_REQUEST_LIMIT);

// TODO: Push group by down to EQS
// If there is a group by, specify a large limit and track actual limit, offset and order by
// expression list, so we can compute these once the we get the results.
if (requestContext.hasGroupBy()) {
// Will need to do the ordering, limit and offset ourselves after we get the group by results
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a bit confused by this comment - aren't we pushing down the order here? Why can't we push down limit/offset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We aren't pushing order by to EDS in case of explore. EDS handler is only called when

  • there is a group by, order by and selections on EDS. In this case, we handle it in memory. We can push group by to EDS along with order by, which is being called out by the TODO comment. That requires a separate effort
  • there is an aggregated selection on EDS (like DISTINCT COUNT of APIs). There is no order by here. And, the limit in this case would be 1 inherently

Copy link
Contributor

Choose a reason for hiding this comment

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

Second case makes sense.

First case - I thought we were pushing down group by (line 61). Discussed offline, but since this is the final query and isn't being joined with anything in memory (right?) we should be able to push everything down unless there's something missing support in EQS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried out different queries. We don't support ordering in document store on function expressions. Since, that's the case, we can't push limit/offset and order bys to EQS

We are already pushing group bys to EQS. Pushing limit and offset to EQS, only if there are no order by expressions (which shouldn't ideally be the case, since group by mostly comes with order by)

orderBy: [
  {
    aggregation: COUNT
    size: null
    direction: DESC
    keyExpression: { key: "name" }
  }
]

builder.setLimit(DEFAULT_ENTITY_REQUEST_LIMIT);
skjindal93 marked this conversation as resolved.
Show resolved Hide resolved
skjindal93 marked this conversation as resolved.
Show resolved Hide resolved
requestContext.setOrderByExpressions(getRequestOrderByExpressions(exploreRequest));
}

return builder.build();
}

Expand Down Expand Up @@ -126,4 +140,9 @@ private Filter.Builder buildFilter(

return filterBuilder.addAllChildFilter(entityIdsInFilter);
}

private List<OrderByExpression> getRequestOrderByExpressions(ExploreRequest request) {
return OrderByUtil.matchOrderByExpressionsAliasToSelectionAlias(
request.getOrderByList(), request.getSelectionList(), request.getTimeAggregationList());
}
}
6 changes: 3 additions & 3 deletions gateway-service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@ plugins {
dependencies {
implementation(project(":gateway-service-factory"))

implementation("org.hypertrace.core.grpcutils:grpc-server-utils:0.12.0")
implementation("org.hypertrace.core.serviceframework:platform-grpc-service-framework:0.1.52")
implementation("org.hypertrace.core.grpcutils:grpc-server-utils:0.12.2")
implementation("org.hypertrace.core.serviceframework:platform-grpc-service-framework:0.1.59")
implementation("org.slf4j:slf4j-api:1.7.30")
implementation("com.typesafe:config:1.4.1")

runtimeOnly("io.grpc:grpc-netty:1.56.0")
runtimeOnly("io.grpc:grpc-netty:1.57.2")
runtimeOnly("org.apache.logging.log4j:log4j-slf4j-impl:2.17.1")
}

Expand Down