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

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

Open
skjindal93 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 9 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
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.58")
api("org.hypertrace.core.serviceframework:platform-grpc-service-framework:0.1.59")

implementation(project(":gateway-service-impl"))
}
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 @@ -122,9 +122,7 @@ private IRequestHandler getRequestHandler(
ExploreRequest request,
Map<String, AttributeMetadata> attributeMetadataMap,
RequestContext requestContext) {
if (isContextAnEntityType(request, requestContext)
&& !hasTimeAggregations(request)
&& !request.getGroupByList().isEmpty()) {
if (isContextAnEntityType(request, requestContext) && !hasTimeAggregations(request)) {
ExpressionContext expressionContext =
new ExpressionContext(
attributeMetadataMap,
Expand Down
24 changes: 16 additions & 8 deletions ...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 @@ -41,19 +41,27 @@
import org.hypertrace.gateway.service.v1.explore.ExploreResponse;

/**
* {@link EntityRequestHandler} is currently used only when the selections, group bys and filters
* are on EDS. Can be extended later to support multiple sources. Only needed, when there is a group
* by on the request, else can directly use {@link
* org.hypertrace.gateway.service.v1.entity.EntitiesRequest}
* {@link EntityRequestHandler} is currently used either
*
* <ul>
* <li>when the selections, group bys and order bys are on EDS. A group by would need an attribute
* selection, aggregation on the same attribute, and order by on any attribute
* <li>when aggregated selection is on EDS. No group by would mean a single aggregated selection
* on attribute
* </ul>
*
* Can be extended later to support multiple sources. Filters can be present both on QS and EDS.
* Only needed, when there is a group by on the request, or an aggregated selection on an attribute,
* else can directly use {@link org.hypertrace.gateway.service.v1.entity.EntitiesRequest}
*
* <p>Currently,
*
* <ul>
* <li>Query to {@link
* org.hypertrace.gateway.service.common.datafetcher.QueryServiceEntityFetcher} with the time
* filter to get set of entity ids. Can be extended to support QS filters
* <li>Query to {@link EntityServiceEntityFetcher} with selections, group bys, and filters with an
* IN clause on entity ids
* filter to get set of entity ids after applying QS filters
* <li>Query to {@link EntityServiceEntityFetcher} with selections(attribute + aggregated), group
* bys(if present), and filters with an IN clause on entity ids
* </ul>
*/
public class EntityRequestHandler extends RequestHandler {
Expand Down Expand Up @@ -94,7 +102,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
21 changes: 21 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,20 @@ private EntityQueryRequest buildRequest(

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

// Ideally, needs the limit and offset for group by, since the fetcher is only triggered when
// there is a group by, or a single aggregation selection. A single aggregated selection would
// always return a single result (i.e. limit 1)
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

@aaron-steinfeld aaron-steinfeld Aug 31, 2023

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

@skjindal93 skjindal93 Oct 13, 2023

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

@aaron-steinfeld aaron-steinfeld Oct 13, 2023

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

@skjindal93 skjindal93 Oct 30, 2023

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" }
  }
]

requestContext.setOrderByExpressions(getRequestOrderByExpressions(exploreRequest));
}

return builder.build();
}

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

return filterBuilder.addAllChildFilter(entityIdsInFilter);
}

private List<OrderByExpression> getRequestOrderByExpressions(ExploreRequest request) {
return OrderByUtil.matchOrderByExpressionsAliasToSelectionAlias(
request.getOrderByList(), request.getSelectionList(), request.getTimeAggregationList());
}
}
2 changes: 1 addition & 1 deletion gateway-service/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ dependencies {
implementation(project(":gateway-service-factory"))

implementation("org.hypertrace.core.grpcutils:grpc-server-utils:0.12.5")
implementation("org.hypertrace.core.serviceframework:platform-grpc-service-framework:0.1.58")
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")

Expand Down
12 changes: 12 additions & 0 deletions owasp-suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,16 @@
<packageUrl regex="true">^pkg:maven/com\.fasterxml\.jackson\.core/jackson\-databind@.*$</packageUrl>
<vulnerabilityName>CVE-2023-35116</vulnerabilityName>
</suppress>
<suppress until="2023-11-30Z">
<notes><![CDATA[
This vulnerability is disputed, with the argument that SSL configuration is the responsibility of the client rather
than the transport. The change in default is under consideration for the next major Netty release, revisit then.
Regardless, our client (which is what brings in this dependency) enables the concerned feature, hostname verification
Ref:
https://github.com/grpc/grpc-java/issues/10033
https://github.com/netty/netty/issues/8537#issuecomment-1527896917
]]></notes>
<packageUrl regex="true">^pkg:maven/io\.netty/netty.*@.*$</packageUrl>
<vulnerabilityName>CVE-2023-4586</vulnerabilityName>
</suppress>
</suppressions>