-
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
Added support for handling complex attribute expression. #107
Conversation
Codecov Report
@@ Coverage Diff @@
## main #107 +/- ##
============================================
+ Coverage 80.17% 81.21% +1.03%
- Complexity 549 573 +24
============================================
Files 53 53
Lines 2134 2204 +70
Branches 225 234 +9
============================================
+ Hits 1711 1790 +79
+ Misses 321 319 -2
+ Partials 102 95 -7
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.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Outdated
Show resolved
Hide resolved
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Outdated
Show resolved
Hide resolved
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Outdated
Show resolved
Hide resolved
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Outdated
Show resolved
Hide resolved
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Outdated
Show resolved
Hide resolved
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Show resolved
Hide resolved
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Outdated
Show resolved
Hide resolved
|
||
@Test | ||
public void testQueryWithGroupByWithMapAttribute() { | ||
Builder builder = QueryRequest.newBuilder(buildGroupByMapAttributeQuery()); |
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 assuming that AttributeExpression without subpath for supporting EQ
and NEQ
will be added as followup, right?
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.
Isn't this case the normal one where such an attribute expression without a subpath represents a normal column identifier?
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.
If you meant CONTAINS_KEY
and CONTAINS_KEYVALUE
support, then yes.
...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java
Outdated
Show resolved
Hide resolved
@@ -37,6 +37,9 @@ | |||
private static final int MAP_KEY_INDEX = 0; | |||
private static final int MAP_VALUE_INDEX = 1; | |||
|
|||
private static final List<Operator> SUPPORTED_OPERATORS_FOR_MAP_ATTRIBUTES = | |||
List.of(Operator.EQ, Operator.NEQ, Operator.GT, Operator.GE, Operator.LT, Operator.LE); |
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.
For Map attribute with sub_path, we will also support LIKE
operator.
Example query for Regex query in Pinot
SELECT mapValue(tags__KEYS, 'span.kind', tags__VALUES) FROM spanEventView WHERE mapValue(tags__KEYS, 'span.kind', tags__VALUES) != '' and REGEXP_LIKE(mapValue(tags__KEYS, 'span.kind', tags__VALUES), '^*.cli*') and start_time_millis > 1636453201000 and start_time_millis < 1636456801000 limit 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.
Will do in a follow up PR.
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.
LIKE is actually the primary ask that kicked off the whole effort, but I'm curious why we need this - are any operators not supported? the whole idea here is that there should be no difference. That was intended for the caller to not care, but I'd imagine the same idea would apply in the code. will read on.
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.
Some of the boolean operators like and/or/not are not appropriate for map attributes.
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.
That's the case for any attribute though, it's not unique to maps. The new commit looks right, but almost all of the other new changes that are are custom to maps should be able to be reverted
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 think we will not need this set for attribute expression having subpath (so for the key of the map). There is support for IN
, NOT IN
as well. So, it will support all the operator ColumnIdentifier was supporting. Can we remove this set? If, any exception that encounter, we can just put that as an exception
e.g
select mapValue(tags__KEYS, 'response_flags', tags__VALUES) from spanEventView
where start_time_millis > 1638497360000 and start_time_millis < 1638508160000
and tags__KEYS = 'response_flags' and mapValue(tags__KEYS, 'response_flags', tags__VALUES) IN ('NR', 'UR')
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.
resolved
String keyCol = convertExpressionToMapKeyColumn(filter.getLhs()); | ||
String valCol = convertExpressionToMapValueColumn(filter.getLhs()); | ||
|
||
if (!SUPPORTED_OPERATORS_FOR_MAP_ATTRIBUTES.contains(filter.getOperator())) { |
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.
Similarly, we also have to check if the mapped attributedId (e.g Span.tags) maps to a complex column in Pinot. Only supported transformation should pass else we need to throw an exception. In our current case, the mapped column should be of type mapFields only - https://github.com/hypertrace/query-service/blob/main/query-service/src/main/resources/configs/common/application.conf#L58
Span.tags
-> is of map type, if not we need to throw an exception.
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.
handleFilterForComplexAttribute
- check if the complex attribute is of map type (mapFields)
- if, yes, handleFilterForMapColumn
handleFilterForMapColumn
- check here that it has supported operators
- if not, throw an exception
- if, yes, do a transformation
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.
Currently, since the only complex attribute is the map attribute, keeping only handleFilterForMapColumn
function
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.
resolved
Should the test in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@@ -269,7 +270,7 @@ private String convertExpressionToString( | |||
viewDefinition.getPhysicalColumnNames(getLogicalColumnName(expression)); | |||
return joiner.join(columnNames); | |||
case ATTRIBUTE_EXPRESSION: | |||
if (isMapAttributeExpression(expression)) { | |||
if (isAttributeExpressionMapAttribute(expression)) { |
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.
Now this condition is correct, just incorrectly named (It's a map selection, not any map attribute).
if ((expression.getValueCase() == COLUMNIDENTIFIER) | ||
|| (expression.getValueCase() == ATTRIBUTE_EXPRESSION && !isComplexAttribute(expression))) { | ||
if (expression.getValueCase() == COLUMNIDENTIFIER | ||
|| isAttributeExpressionMapAttribute(expression)) { |
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're making our own utils here, so ideally should be encapsulating this in a single predicate. More importantly however, this would fail on an attribute expression without a subpath. If we want to remove the type check here, there's no reason to even include an if condition - the thrown exception is saying it's a type mismatch.
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.
In other words we can proceed one of two ways:
- Fix
isAttributeExpressionMapAttribute
to take in a view def and ensure it's a map, independent of subpath - Remove this check and the thrown exception and run this code unconditionally
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.
Earlier didn't remove as it was already present in the main before all these changes. Removed 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.
resolved
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
public static boolean isComplexAttribute(Expression expression) { | ||
return expression.getValueCase().equals(ATTRIBUTE_EXPRESSION) | ||
&& expression.getAttributeExpression().hasSubpath(); | ||
public static Filter createContainsKeyFilter(String column, String 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.
nit: can be inline above function or can be private as it (createContainsKeyFilter(String column, String value)
is only used in Tests now. But, can be taken as follow up.
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.
overall, now lgtm. But, see @aaron-steinfeld any open comments before merging this.
} | ||
throw new IllegalArgumentException( | ||
"operator CONTAINS_KEY/KEYVALUE supports multi value column only"); | ||
throw new IllegalArgumentException("operator supports multi value column only"); |
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.
Follow up is fine but we should fixup this error message since the type check is gone. Now, this error is reached if no key column exists for the provided attribute - that could be an attribute that's mistyped, but it also may just not be a column in that view.
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.
So what should be the error here 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.
Unable to lookup key column for attribute: {}
, or something like that
Builder builder = QueryRequest.newBuilder(); | ||
Expression spanTag = createSimpleAttributeExpression("Span.tags").build(); | ||
Expression spanTag = createComplexAttributeExpression("Span.tags", "FLAGS").build(); |
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.
does our code lowercase this? I would imagine we should respect the key's casing, unless we're lowercasing the data as we persist it.
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.
This is interesting, need to check. But, it seems that as prior existing tests (for CONTAINS_KEYVALUE) were also changed to lower case.
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.
Yeah, we should follow up on this. May be working correctly, but it caught my eye. If we're pushing everything to lowercase on ingestion side, we have to do this; if we're not, I'd consider it a bug.
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.
No, we are not doing anything specific for lowercase. It is the assertion utility in the test were written as lowercase for comparing two string for expected and actual string.
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.
Ugh, we should remove that. Case matters in (certain positions of) a query, and this is testing the query itself.
query-service-impl/src/test/java/org/hypertrace/core/query/service/pinot/MigrationTest.java
Show resolved
Hide resolved
} | ||
|
||
private QueryRequest buildGroupByMapAttributeQuery() { | ||
Builder builder = QueryRequest.newBuilder(); |
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: For test inputs that are this complex, it's usually more readable and a better test (since it better mimics an upstream client) to provide them as text.
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 didn't get you
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 building the query in code, this would mean deserializing it from text - either an inline string or a file.
Ref issue : #105