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

saxenakshitiz · 2023-12-06T12:15:46Z

No description provided.

github-actions · 2023-12-06T12:17:07Z

Test Results

364 tests +3 364 ✔️ +3 8s ⏱️ ±0s
  66 suites ±0     0 💤 ±0
  66 files ±0     0 ❌ ±0

Results for commit a17bd2b. ± Comparison against base commit 9f2aa9d.

This pull request removes 29 and adds 17 tests. Note that renamed tests count towards both.


      long: 60
      string: "PT1M"
      valueType: LONG
      valueType: STRING
    alias: "numCalls"
    columnName: "SERVICE.numCalls"
    value {
    }
  columnIdentifier {
…

org.hypertrace.gateway.service.baseline.BaselineServiceImplTest ‑ [1] function: AVGRATE
arguments {
  columnIdentifier {
    columnName: "SERVICE.numCalls"
    alias: "numCalls"
  }
}
arguments {
  literal {
    value {
      valueType: STRING
      string: "PT1M"
    }
  }
}
alias: "numCalls"

org.hypertrace.gateway.service.baseline.BaselineServiceImplTest ‑ [2] function: AVGRATE
arguments {
  columnIdentifier {
    columnName: "SERVICE.numCalls"
    alias: "numCalls"
  }
}
arguments {
  literal {
    value {
      valueType: LONG
      long: 60
    }
  }
}
alias: "numCalls"

org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [10] group-by-with-offset
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [11] simple-aggregations
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [12] group-by-with-group-limit-and-rest
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [13] grouped-time-aggregation-with-group-limit-with-null-value-group
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [14] time-aggregations
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [15] aggregations-with-multiple-groupbys-and-the-rest-deprecated
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [1] grouped-time-aggregation-with-group-limit-with-null-and-nonnull-value-group
org.hypertrace.gateway.service.explore.ExploreServiceTest ‑ [2] time-aggregations-space-filtered
…

♻️ This comment has been updated with latest results.

codecov · 2023-12-06T12:17:37Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (9f2aa9d) 81.48% compared to head (a17bd2b) 81.47%.

Files	Patch %	Lines
...ay/service/explore/TheRestGroupRequestHandler.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main     #186      +/-   ##
============================================
- Coverage     81.48%   81.47%   -0.01%     
- Complexity     1329     1334       +5     
============================================
  Files           125      125              
  Lines          5963     5971       +8     
  Branches        490      490              
============================================
+ Hits           4859     4865       +6     
- Misses          863      864       +1     
- Partials        241      242       +1

Flag	Coverage Δ
unit	`81.47% <80.00%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

aaron-steinfeld · 2023-12-06T19:04:16Z

...in/java/org/hypertrace/gateway/service/common/converters/StringToAttributeKindConverter.java

@@ -92,6 +94,12 @@ private List<String> convertToArray(String jsonString) {
      return List.of();
    }

+    // handle special case when "null" string is returned as string array value(default value


Discussed offline - concerned about this behavior change, as every query that was previously returning an empty array (e.g. give me the labels for this API) will now start containing nulls. Existing behavior seemed more accurate, we just need to make sure we appropriately handle group by case and either return no groups (short circuit) or translate it back if we think we should look for explicitly empty groups as part of a group by query. I'm not sure what the actual correct expectation should be there.

As you mentioned, the Gateway (GW) is not handling null (or empty) translations, especially for string_array attributes on both sides (input/output). While outputting, it treats null from downstream as an empty list, but when making a query, it does not do so. The GW should handle to its own definition of handling null (or empty) when sending requests to downstream translation as well.

So, would suggest that when converting a query to Query Service (QS), if there is a string_array attribute and its right-hand side (RHS) value is empty (empty list), it should be translated to null before sending query to QS.

@saxenakshitiz Sorry, was in a hurry yesterday when suggesting to output null in the toArrayConvert method. This makes sense - link to the GitHub discussion.

aaron-steinfeld · 2023-12-06T22:57:02Z

...n/java/org/hypertrace/gateway/service/explore/TimeAggregationsWithGroupByRequestHandler.java

+        // 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()) {


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

Filtering is actually introducing rather than removing results

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)?

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.

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.

Agreed that both should return empty list

inadvertent approval

...n/java/org/hypertrace/gateway/service/explore/TimeAggregationsWithGroupByRequestHandler.java

...ce-impl/src/main/java/org/hypertrace/gateway/service/explore/TheRestGroupRequestHandler.java

kotharironak

LGTM.

kotharironak · 2023-12-07T16:56:14Z

...-test-responses/explore/grouped-time-aggregation-with-group-limit-with-null-value-group.json

@@ -0,0 +1,45 @@
+{
+  "row": [{


Whats the query look like for this for gateway?

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

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.

We have all queries in the test

GraphQL query to GW

GW query to QS and its response

GW response to GraphQL

...ce-impl/src/main/java/org/hypertrace/gateway/service/explore/TheRestGroupRequestHandler.java

aaron-steinfeld · 2023-12-07T17:23:02Z

...ce-impl/src/main/java/org/hypertrace/gateway/service/common/converters/QueryRequestUtil.java

@@ -127,4 +128,17 @@ public static Expression createTimeColumnGroupByExpression(
                .addArguments(createStringLiteralExpression(periodSecs + ":SECONDS")))
        .build();
  }
+
+  public static List<String> convertStringArrayValue(


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.

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

gateway-service-impl/src/test/resources/attributes/attributes.json

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

da7e1ec

saxenakshitiz requested a review from a team as a code owner December 6, 2023 12:15

saxenakshitiz added 2 commits December 6, 2023 17:48

minor changes

5336c3b

fix dependency check

61750ec

aaron-steinfeld reviewed Dec 6, 2023

View reviewed changes

address review comments

8cb1ea6

aaron-steinfeld previously approved these changes Dec 6, 2023

View reviewed changes

added tests and handled others case

44bf5bc

kotharironak reviewed Dec 7, 2023

View reviewed changes

...n/java/org/hypertrace/gateway/service/explore/TimeAggregationsWithGroupByRequestHandler.java Outdated Show resolved Hide resolved

remove response update

cb44fd5

kotharironak reviewed Dec 7, 2023

View reviewed changes

...ce-impl/src/main/java/org/hypertrace/gateway/service/explore/TheRestGroupRequestHandler.java Show resolved Hide resolved

...ce-impl/src/main/java/org/hypertrace/gateway/service/explore/TheRestGroupRequestHandler.java Outdated Show resolved Hide resolved

address review comments

76e4bb4

kotharironak reviewed Dec 7, 2023

View reviewed changes

aaron-steinfeld reviewed Dec 7, 2023

View reviewed changes

address review comments

53737a6

aaron-steinfeld reviewed Dec 7, 2023

View reviewed changes

gateway-service-impl/src/test/resources/attributes/attributes.json Outdated Show resolved Hide resolved

address review comments

a17bd2b

aaron-steinfeld approved these changes Dec 7, 2023

View reviewed changes

saxenakshitiz merged commit 8f4dfb1 into main Dec 7, 2023

saxenakshitiz deleted the handle_null_string_array_value branch December 7, 2023 19:03

kotharironak mentioned this pull request Dec 11, 2023

fix: handles default case for string array IN filter for pinot handlers hypertrace/query-service#215

Open

3 tasks

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

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

Uh oh!

Conversation

saxenakshitiz commented Dec 6, 2023

Uh oh!

github-actions bot commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

codecov bot commented Dec 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotharironak Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kotharironak Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kotharironak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 6, 2023 •

edited

Loading

codecov bot commented Dec 6, 2023 •

edited

Loading

kotharironak Dec 7, 2023 •

edited

Loading

kotharironak Dec 7, 2023 •

edited

Loading