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
saxenakshitiz 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 4 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
11 changes: 10 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
Expand Up @@ -30,6 +30,8 @@ public class TimeAggregationsWithGroupByRequestHandler implements IRequestHandle
private static final Logger LOG =
LoggerFactory.getLogger(TimeAggregationsWithGroupByRequestHandler.class);

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

private final AttributeMetadataProvider attributeMetadataProvider;
private final RequestHandler normalRequestHandler;
private final TimeAggregationsRequestHandler timeAggregationsRequestHandler;
Expand Down Expand Up @@ -228,7 +230,14 @@ private void buildInClauseFilterValue(Value value, Value.Builder valueBuilder) {
valueBuilder.addStringArray(value.getString());
break;
case STRING_ARRAY:
valueBuilder.addAllStringArray(value.getStringArrayList());
// Handle special case when "null" string is returned as string array value(default value
// scenario). This will be converted to empty list in value converter. In such a case use
// list with "null" value
if (value.getStringArrayList().isEmpty()) {
Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Dec 6, 2023

Choose a reason for hiding this comment

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

so one behavior discrepancy this would lead to to - let's say we have 10 rows, and 9 of them have no values for a multivalued col X and 1 of them does. Both before and after this change, if we grouped by X we'd get a single returned grouped with a count of 1. Now, let's say we filter for where X = null. Before this change, we'd error out (the in [] case we're trying to fix), after we'd return a new group of [] -> 9. So this is problematic for two different reasons

  1. Filtering is actually introducing rather than removing results
  2. There's a group that's only discoverable when no other values are present.

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

Copy link
Contributor

@kotharironak kotharironak Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

so one behavior discrepancy this would lead to to - let's say we have 10 rows, and 9 of them have no values for a multivalued col X and 1 of them does. Both before and after this change, if we grouped by X we'd get a single returned grouped with a count of 1.

From the Query Service (QS) point of view, it will return two groups. Let's assume that 1 row which is not empty has value - ["hello"]. The output will be as below.
Returned groups:

{
  {"null", 9},
  {"hello", 1}
}

And if the none empty row of X has more than one value, say - ["hello", "world"], QS will return 3 groups:

{
  {"null", 9},
  {"hello", 1},
  {"world", 1}
}

Moving on to Pinot, if the column (or multi-value column) has no value, it will return the default value. The default value for a String column is null. For a multi-value column, if not preset, it uses the default of a single-value column of the same type.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Dec 7, 2023

Choose a reason for hiding this comment

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

The described behavior makes sense, the first issue to resolve however is how GW should behave because it's not consistent. If we do a group by with time series, it won't return that null (or empty list); if we do it without, it returns an empty list as one value.

Copy link
Contributor

@aaron-steinfeld aaron-steinfeld Dec 7, 2023

Choose a reason for hiding this comment

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

Agreed that both should return empty list

valueBuilder.addAllStringArray(LIST_WITH_NULL_STRING);
} else {
valueBuilder.addAllStringArray(value.getStringArrayList());
}
break;
case LONG:
valueBuilder.addLongArray(value.getLong());
Expand Down
19 changes: 10 additions & 9 deletions owasp-suppressions.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,7 @@
<cpe>cpe:/a:utils_project:utils</cpe>
<cpe>cpe:/a:service_project:service</cpe>
</suppress>
<suppress until="2023-07-31Z">
<notes><![CDATA[
Doesn't appear to be a real vulnerability, jackson maintainers discuss at https://github.com/FasterXML/jackson-databind/issues/3973
Revisit when suppression expires
]]></notes>
<packageUrl regex="true">^pkg:maven/com\.fasterxml\.jackson\.core/jackson\-databind@.*$</packageUrl>
<vulnerabilityName>CVE-2023-35116</vulnerabilityName>
</suppress>
<suppress until="2023-11-30Z">
<suppress until="2023-12-31Z">
<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.
Expand All @@ -29,4 +21,13 @@
<packageUrl regex="true">^pkg:maven/io\.netty/netty.*@.*$</packageUrl>
<vulnerabilityName>CVE-2023-4586</vulnerabilityName>
</suppress>
<suppress until="2023-12-31Z">
<notes><![CDATA[
This CVE (rapid RST) is already mitigated as our servers aren't directly exposed, but it's also
addressed in 1.59.1, which the CVE doesn't reflect (not all grpc impls versions are exactly aligned).
Ref: https://github.com/grpc/grpc-java/pull/10675
]]></notes>
<packageUrl regex="true">^pkg:maven/io\.grpc/grpc\-.*@.*$</packageUrl>
<cve>CVE-2023-44487</cve>
</suppress>
</suppressions>