-
Notifications
You must be signed in to change notification settings - Fork 10
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
Adding support for IN and NOT IN for non-string array values #207
Conversation
Codecov Report
@@ Coverage Diff @@
## main #207 +/- ##
============================================
+ Coverage 72.97% 73.17% +0.20%
+ Complexity 1097 10 -1087
============================================
Files 96 96
Lines 4781 4780 -1
Branches 550 547 -3
============================================
+ Hits 3489 3498 +9
+ Misses 1065 1055 -10
Partials 227 227
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today! |
This comment has been minimized.
This comment has been minimized.
assertSQLQuery( | ||
buildSimpleQueryWithFilter( | ||
createLongInFilter("Span.metrics.duration_millis", List.of(10L))), | ||
"Select encode(span_id, 'hex') FROM public.\"span-event-view\" WHERE " |
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.
I am not able to recollect, why we didn't add that support prior?
Can you once try these queries on Pinot for validation? If you have already tried, we can ignore 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.
Verified the query
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.
LGTM
case LONG_ARRAY: | ||
ret = buildArrayValue(value.getLongArrayList(), paramsBuilder::addLongParam); | ||
break; | ||
case INT_ARRAY: |
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.
@satish-mittal FYI: we might have to add the support for trino too. But, we also have to check if those queries works fine first.
+ " = '" | ||
+ TENANT_ID | ||
+ "' " | ||
+ "AND duration_millis IN (10)", |
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.
nit: will be good to have some tests using IN clause with multiple values.
+ " = '" | ||
+ TENANT_ID | ||
+ "' " | ||
+ "AND duration_millis NOT IN (10.05)", |
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.
Instead of reusingduration_millis
field for all types that don't apply to it, it's cleaner to use respective fields for various types. e.g. latitude/longitude for float/double. The fields need to be just added in request_handler.conf.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@varsha-abhinandan both the vulnerabilities detected are disputed. Look for other places where it has been suppressed. https://github.com/hypertrace/hypertrace-graphql/blob/d5c2a3eeed827ce56fb8e9f453f9c1767271473e/owasp-suppressions.xml#L30 |
This comment has been minimized.
This comment has been minimized.
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.
Had reviewed once, recently there are test case changes. It lgtm.
Description
Please include a summary of the change, motivation and context.
Testing
Please describe the tests that you ran to verify your changes. Please summarize what did you test and what needs to be tested e.g. deployed and tested helm chart locally.
Checklist:
Documentation
Make sure that you have documented corresponding changes in this repository or hypertrace docs repo if required.