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

chore: convert 'null' string array value to list with 'null' value #186

Merged
merged 9 commits into from
Dec 7, 2023
Merged

chore: convert 'null' string array value to list with 'null' value #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
da7e1ec
chore: convert 'null' string array value to list with 'null' value
Dec 6, 2023
5336c3b
minor changes
Dec 6, 2023
61750ec
fix dependency check
Dec 6, 2023
8cb1ea6
address review comments
Dec 6, 2023
44bf5bc
added tests and handled others case
Dec 7, 2023
cb44fd5
remove response update
Dec 7, 2023
76e4bb4
address review comments
Dec 7, 2023
53737a6
address review comments
Dec 7, 2023
a17bd2b
address review comments
Dec 7, 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-impl/build.gradle.kts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,8 @@ dependencies {
implementation("com.google.guava:guava:32.1.2-jre")
implementation("com.google.inject:guice:5.0.1")

implementation("com.fasterxml.jackson.core:jackson-annotations:2.15.2")
implementation("com.fasterxml.jackson.core:jackson-databind:2.15.2")
implementation("com.fasterxml.jackson.core:jackson-annotations:2.16.0")
implementation("com.fasterxml.jackson.core:jackson-databind:2.16.0")

testImplementation("org.junit.jupiter:junit-jupiter:5.8.2")
testImplementation("org.mockito:mockito-junit-jupiter:5.4.0")
Expand Down
16 changes: 16 additions & 0 deletions ...impl/src/main/java/org/hypertrace/gateway/service/common/converters/QueryRequestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ public class QueryRequestUtil {

private static final String COUNT_FUNCTION_NAME = "COUNT";
private static final String DISTINCTCOUNT_FUNCTION_NAME = "DISTINCTCOUNT";
private static final List<String> LIST_WITH_NULL_STRING = List.of("null");

public static Filter createBetweenTimesFilter(String columnName, long lower, long higher) {
return Filter.newBuilder()
Expand Down Expand Up @@ -127,4 +128,19 @@ public static Expression createTimeColumnGroupByExpression(
.addArguments(createStringLiteralExpression(periodSecs + ":SECONDS")))
.build();
}

public static List<String> convertStringArrayValue(
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we adjust this naming and maybe put it with the conversion in the other direction (is that in this file?) - it's usage and implementation make sense, but the naming still feels very unintuitive. Not required if this introduces and meaningful refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not refactoring. Added a link to method which handles conversion in other direction.

org.hypertrace.gateway.service.v1.common.Value value) {
// The downstream QS service returns the default value "null" for a string array when it is
// empty. GW returns an empty list as output. This transformation occurs in the value converter.
// To handle this scenario when querying downstream with an empty list, set its default value to
// "null."
// Refer to {@linkplain StringToAttributeKindConverter#convertToArray} to check how QS response
// for string array column is converted
if (value.getStringArrayList().isEmpty()) {
return LIST_WITH_NULL_STRING;
} else {
return value.getStringArrayList();
}
}
}
16 changes: 15 additions & 1 deletion ...impl/src/main/java/org/hypertrace/gateway/service/explore/TheRestGroupRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
package org.hypertrace.gateway.service.explore;

import static org.hypertrace.gateway.service.common.converters.QueryRequestUtil.convertStringArrayValue;

import com.google.common.collect.ImmutableSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.function.Function;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.hypertrace.gateway.service.common.util.ExpressionReader;
import org.hypertrace.gateway.service.v1.common.Expression;
import org.hypertrace.gateway.service.v1.common.Filter;
Expand Down Expand Up @@ -224,10 +227,21 @@ private Set<String> getExcludedValues(
String resultName, ExploreResponse.Builder originalResponse) {
return originalResponse.getRowBuilderList().stream()
.map(rowBuilder -> rowBuilder.getColumnsMap().get(resultName))
.map(Value::getString)
.flatMap(this::getStringValues)
.collect(ImmutableSet.toImmutableSet());
}

private Stream<String> getStringValues(Value value) {
switch (value.getValueType()) {
case STRING:
return Stream.of(value.getString());
case STRING_ARRAY:
return convertStringArrayValue(value).stream();
default:
return Stream.of();
}
}

private Filter.Builder createExcludedChildFilter(
Expression groupBySelectionExpression, Set<String> excludedValues) {
return Filter.newBuilder()
Expand Down
4 changes: 3 additions & 1 deletion ...ava/org/hypertrace/gateway/service/explore/TimeAggregationsWithGroupByRequestHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.hypertrace.gateway.service.explore;

import static org.hypertrace.gateway.service.common.converters.QueryRequestUtil.convertStringArrayValue;

import com.google.common.collect.Streams;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -228,7 +230,7 @@ private void buildInClauseFilterValue(Value value, Value.Builder valueBuilder) {
valueBuilder.addStringArray(value.getString());
break;
case STRING_ARRAY:
valueBuilder.addAllStringArray(value.getStringArrayList());
valueBuilder.addAllStringArray(convertStringArrayValue(value));
break;
case LONG:
valueBuilder.addLongArray(value.getLong());
Expand Down
11 changes: 11 additions & 0 deletions gateway-service-impl/src/test/resources/attributes/attributes.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -945,6 +945,17 @@
"QS"
]
},
{
"fqn": "Service.labels",
"valueKind": "TYPE_STRING_ARRAY",
"key": "labels",
"displayName": "Service Labels",
"scopeString": "SERVICE",
"type": "ATTRIBUTE",
"sources": [
"QS"
]
},
{
"fqn": "Service.start_time_millis",
"valueKind": "TYPE_INT64",
Expand Down
98 changes: 98 additions & 0 deletions ...explore/grouped-time-aggregation-with-group-limit-and-the-rest-with-null-value-group.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
{
"row": [
{
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615593600000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1616"
}
}
},
{
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615593600000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY",
"string_array": ["label1"]
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1989"
}
}
},
{
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615597200000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "2525"
}
}
},
{
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615597200000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY",
"string_array": ["label1"]
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1977"
}
}
},
{
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615593600000"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "27485"
},
"SERVICE.labels": {
"valueType": "STRING",
"string": "__Other"
}
}
},
{
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615597200000"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "27192"
},
"SERVICE.labels": {
"valueType": "STRING",
"string": "__Other"
}
}
}
]
}
90 changes: 90 additions & 0 deletions .../explore/grouped-time-aggregation-with-group-limit-with-null-and-nonnull-value-group.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
{
"row": [{
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615593600000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1616"
}
}
}, {
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615593600000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY",
"string_array": ["label1"]
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1989"
}
}
}, {
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615597200000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "2525"
}
}
}, {
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615597200000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY",
"string_array": ["label1"]
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1977"
}
}
}, {
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615600800000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY",
"string_array": ["label1"]
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1965"
}
}
}, {
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615600800000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "2323"
}
}
}]
}
45 changes: 45 additions & 0 deletions ...st-responses/explore/grouped-time-aggregation-with-group-limit-with-null-value-group.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{
"row": [{
Copy link
Contributor

Choose a reason for hiding this comment

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

Whats the query look like for this for gateway?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! okey, it will be an internal query. Or would be IN query from GraphQL to gateway too. That is currently not expressible, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

So, if a gateway client needs to explicitly ask for a null group, it should create a filter with the right-hand side (RHS) containing only the valueType. However, it can't query the null group along with other groups explicitly in the IN clause, right? This is same refer in the comment by @aaron-steinfeld here - #186 (comment)

WDYT about just always adding null here (if not already present)?

This will be opposite to the current case - where we don't want the null group; then we will always return it. I think both seem the same to me. So, I would say we can stick to the current behavior here.

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 have all queries in the test

  1. GraphQL query to GW
  2. GW query to QS and its response
  3. GW response to GraphQL

"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615593600000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "1616"
}
}
}, {
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615597200000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "2525"
}
}
}, {
"columns": {
"INTERVAL_START_TIME": {
"valueType": "LONG",
"long": "1615600800000"
},
"SERVICE.labels": {
"valueType": "STRING_ARRAY"
},
"SUM_SERVICE.numCalls_[]": {
"valueType": "LONG",
"long": "2323"
}
}
}]
}
Loading