fix: support for attribute expressions in selections #129
Conversation
Codecov Report
@@ Coverage Diff @@
## main #129 +/- ##
============================================
+ Coverage 81.25% 81.39% +0.14%
- Complexity 574 588 +14
============================================
Files 53 58 +5
Lines 2235 2268 +33
Branches 236 234 -2
============================================
+ Hits 1816 1846 +30
- Misses 325 327 +2
- Partials 94 95 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…selections-refactor
...e/query/service/attribubteexpression/AttributeExpressionNormalizationTransformationTest.java
Outdated
Show resolved
Hide resolved
Co-authored-by: sarthak <sarthak02singhal@gmail.com>
This comment has been minimized.
This comment has been minimized.
.../core/query/service/attribubteexpression/AttributeExpressionNormalizationTransformation.java
Show resolved
Hide resolved
...e/query/service/attribubteexpression/AttributeExpressionNormalizationTransformationTest.java
Show resolved
Hide resolved
| if (filter.equals(Filter.getDefaultInstance())) { | ||
| return Single.just(filter); | ||
| private Expression addSubpathOrThrow(Expression projectedExpression, String subpath) { | ||
| if (!QueryRequestUtil.isSimpleAttributeExpression(projectedExpression)) { |
There was a problem hiding this comment.
Can you give one example or add a test for this when we hit this condition?
A projected attribute is already having subpath?
There was a problem hiding this comment.
Yeah, will add a test. As a quick example, imagine if we rename a column and so use a simple projection to point one attribute name at another. In this hypothetical, an old name of EVENT.spanTags becomes EVENT.attributes. An arriving attribute expression may still be using the old name, so when we project
"attributeExpression": {
"attributeId": "EVENT.spanTags",
"subpath": "span.kind"
}the projection is only based on the attribute itself, so above we first project:
"attributeExpression": {
"attributeId": "EVENT.attributes"
}then this function is meant to restore the subpath part giving us our final projection:
"attributeExpression": {
"attributeId": "EVENT.attributes",
"subpath": "span.kind"
}It currently throws if the projection did not result in another simple attribute expression, since if the result was already a complex expression (either a function or map selection), it would be more difficult/potentially impossible to support (to be honest, I didn't go too far down that path - we don't have that use case at the moment and can cross that bridge when/if we get there).
There was a problem hiding this comment.
Test added in #132
…:hypertrace/query-service into attribute-expression-selections-refactor
| @@ -226,6 +228,35 @@ public void testQueryWithNotContainsKeyOperator() { | |||
| executionContext); | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
This comment has been minimized.
This comment has been minimized.
| @@ -96,8 +96,7 @@ public static void setup() throws Exception { | |||
| new GenericContainer<>(DockerImageName.parse("hypertrace/pinot-servicemanager:main")) | |||
| .withNetwork(network) | |||
| .withNetworkAliases("pinot-controller", "pinot-server", "pinot-broker") | |||
| .withExposedPorts(8099) | |||
| .withExposedPorts(9000) | |||
| .withExposedPorts(8099, 9000) | |||
There was a problem hiding this comment.
This was a pre-existing bug with test config, the second one overwrites the first, it takes varargs instead. It was previously working due to a bug in test containers that was fixed when we upgraded to fix vulnerable deps (unexposed ports on the container were still exposed).
This comment has been minimized.
This comment has been minimized.
| @@ -24,6 +24,7 @@ | |||
| QueryTransformationContext transformationContext = | |||
| new DefaultQueryTransformationContext(tenantId); | |||
| return Observable.fromIterable(transformations) | |||
| .sorted() | |||
There was a problem hiding this comment.
As part of this PR, we put in the concept of ordered transformations, so this ensures their order (since mutli injection does not).
This comment has been minimized.
This comment has been minimized.
|
@aaron-steinfeld let me know if we are upgrading to log4j version? I will re-approve. |
|
Will upgrade it in a following PR |
|
Description
Testing