-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handles default case for string array IN filter for pinot handlers #215
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #215 +/- ##
=========================================
Coverage 73.26% 73.27%
- Complexity 1115 1116 +1
=========================================
Files 96 96
Lines 4784 4785 +1
Branches 547 548 +1
=========================================
+ Hits 3505 3506 +1
Misses 1054 1054
Partials 225 225
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
This comment has been minimized.
This comment has been minimized.
@@ -450,7 +450,9 @@ private String convertLiteralToString(LiteralConstant literal, Params.Builder pa | |||
String ret = null; | |||
switch (value.getValueType()) { | |||
case STRING_ARRAY: | |||
ret = buildArrayValue(value.getStringArrayList(), paramsBuilder::addStringParam); | |||
List<String> values = | |||
value.getStringArrayList().size() > 0 ? value.getStringArrayList() : List.of("null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is gateway service converting null string to empty ? Shouldn't be fix that instead of expecting every converter to handle this ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'null' is special case on how it is stored in Pinot and needs to be converted to an appropriate value. Isn't it?
@@ -450,7 +450,9 @@ private String convertLiteralToString(LiteralConstant literal, Params.Builder pa | |||
String ret = null; | |||
switch (value.getValueType()) { | |||
case STRING_ARRAY: | |||
ret = buildArrayValue(value.getStringArrayList(), paramsBuilder::addStringParam); | |||
List<String> values = | |||
value.getStringArrayList().size() > 0 ? value.getStringArrayList() : List.of("null"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are we handling special case only for string array? There are other array types as well. We don't need to handle this case for them?
Closing this with respect : hypertrace/gateway-service#186 |
Description
The default value for string column is
null
in pinot - https://docs.pinot.apache.org/configuration-reference/schemaSo, there are cases when query can only returns
null
group. These cases a follow up query with empty clause as shown in the below example:It could possible that given time limit window, we have only one group
null
.This PR, handles the above default scenario.
Testing
Added Unit test.
Checklist: