-
Notifications
You must be signed in to change notification settings - Fork 7
Filtering pagination support impl #286
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
Conversation
|
|
||
| // optional - pagination parameters to limit and offset the result set. | ||
| // Useful for retrieving configs in pages when total count is large. | ||
| Pagination pagination = 5; |
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.
Removed total_count for now, will add it later if required
config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
Show resolved
Hide resolved
...vice-impl/src/main/java/org/hypertrace/config/service/store/ConstantExpressionConverter.java
Outdated
Show resolved
Hide resolved
| return ConstantExpression.of(value.getBoolValue()); | ||
| case LIST_VALUE: | ||
| List<Value> values = value.getListValue().getValuesList(); | ||
| // Infer and cast list element types (assumes homogeneous list) |
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.
Rather than assume it and leave a comment we can easily validate this. e.g. collect kinds to set and ensure == 1. Or really <= 1, why not support empty list? Seems trivial to do and there are reasonable use cases (e.g. my_array_field EQ [])
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.
Fixed it
|
|
||
| case STRUCT_VALUE: | ||
| throw new UnsupportedOperationException("Struct not supported directly in this context"); | ||
| case NULL_VALUE: |
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.
We can't compare to 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.
Not able to find support Null values in ConstantExpression. Thats why marked as unsupported for now.
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.
OK, we should add that at some point. For now, the main use case is probably eq/neq NULL which would represent in doc store as a EXISTS or NOT_EXISTS. So we could consider doing that translation if we need it rather than introducing those extra operators.
...-service-impl/src/main/java/org/hypertrace/config/service/store/FilterExpressionBuilder.java
Outdated
Show resolved
Hide resolved
config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
Show resolved
Hide resolved
config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
Outdated
Show resolved
Hide resolved
config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
Outdated
Show resolved
Hide resolved
config-service-impl/src/main/java/org/hypertrace/config/service/store/DocumentConfigStore.java
Show resolved
Hide resolved
config-service-impl/src/test/java/org/hypertrace/config/service/ConfigServiceGrpcImplTest.java
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #286 +/- ##
============================================
- Coverage 79.93% 76.92% -3.02%
- Complexity 496 556 +60
============================================
Files 55 62 +7
Lines 2432 2795 +363
Branches 108 131 +23
============================================
+ Hits 1944 2150 +206
- Misses 427 563 +136
- Partials 61 82 +21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| return ConstantExpression.ofStrings(List.of()); | ||
| } | ||
|
|
||
| // Infer and cast list element types (assumes homogeneous list) |
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.
See earlier comment. We can easily validate this assumption and throw rather than translate something different than the caller intended
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.
Fixed it. Please check.
Description
Add support for Filtering, pagination and sorting in Config Service GetAllConfigsAPI