Fix : Add support for contains key and like operator #125

sarthak77 · 2021-12-15T04:08:16Z

Ref issue : #122 , #109

codecov · 2021-12-15T04:09:38Z

Codecov Report

Merging #125 (de8f2ce) into main (392d36b) will increase coverage by 0.09%.
The diff coverage is 92.85%.

@@             Coverage Diff              @@
##               main     #125      +/-   ##
============================================
+ Coverage     81.06%   81.16%   +0.09%     
  Complexity      568      568              
============================================
  Files            53       53              
  Lines          2192     2193       +1     
  Branches        235      235              
============================================
+ Hits           1777     1780       +3     
+ Misses          321      318       -3     
- Partials         94       95       +1

Flag	Coverage Δ
integration	`81.16% <92.85%> (+0.09%)`	⬆️
unit	`79.03% <92.85%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...service/pinot/QueryRequestToPinotSQLConverter.java	`91.16% <91.30%> (+0.07%)`	⬆️
...ypertrace/core/query/service/QueryRequestUtil.java	`71.42% <100.00%> (-0.45%)`	⬇️
.../query/service/pinot/PinotBasedRequestHandler.java	`71.05% <0.00%> (+0.32%)`	⬆️
...rtrace/core/query/service/QueryServiceStarter.java	`58.33% <0.00%> (+2.77%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 392d36b...de8f2ce. Read the comment docs.

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java

aaron-steinfeld · 2021-12-15T15:14:20Z

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java

+          && value.getValue().getStringArrayCount() == 2) {
+        literals.set(0, value.getValue().getStringArray(0));
+        literals.set(1, value.getValue().getStringArray(1));
+      } else if (value.getValue().getValueType() == ValueType.STRING) {


So support for a string arg is the only functional change in this PR, right?

Mostly nit changes till now. I thought like and contains_key will not work but with the new changes, they were working so just added unit tests for them till now.

aaron-steinfeld · 2021-12-15T15:17:41Z

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java

-  private LiteralConstant[] convertExpressionToMapLiterals(Expression expression) {
-    LiteralConstant[] literals = new LiteralConstant[2];
+  private List<LiteralConstant> convertExpressionToMapLiterals(Expression expression) {
+    List<String> literals = new ArrayList<>(List.of("", ""));


If we're going to clean up the implementation, let's do it right and return an object rather than a list.

@Value class MapKeyValueArguments { LiteralConstant key; Optional<LiteralConstant> value; }

can Map.Entry<LiteralConstant, Optional<LiteralConstant>> be used as tuple?

I'm a bit on the fence. I probably wouldn't do it, but I wouldn't call it out on review. In general, we'd rather create a new class than use an Entry or Pair class, but in this case it's reasonable to use Entry since we're representing a key and value (Pair pretty much never makes sense, because left and right have no meaning). The reason I'd still lean against it is that we're kind of mixing a structural paradigm with an object-oriented one. What we're producing here "is a" map entry, but it would never be used in a map - we're just using an existing unrelated class because it has the structure we're looking for.

If the key is an index like 0 or 1, then why do we need it to be a literalConstant ? Then at the places where we access using 0/1, we would need to create a literalConstant first.

@sarthak77 I didn't fully understand - Then at the places where we access using 0/1, we would need to create a literalConstant first.

But, the proposal of https://github.com/hypertrace/query-service/pull/125/files#r769725462, makes it more readable and clear the explicit indexing based usage - x[0] or x.get(0).

Note: This will change a bit once you add support for handling attribute expression in all the places. E.g the function convertExpressionToMapLiterals will need lhs expression get the key.

But, if we are modifying from the existing way, I would also suggest having a readable structure the way @aaron-steinfeld has suggested.

Then also it will still have rhs right ?
Like we can say Span.tags CONTAINS_KEY 'flags' or lhs CONTAINS_KEY rhs

Yes, you are right. Rhs is a key in map. I got confused.

How does the int key value be used? How will we get LiteralConsant, will we still have List? Based on aaron's suggestion, you can do something as below. And, you can access key literal constant directly from the object. e.g builder.append(convertLiteralToString(mapKeyValueArguments.getKey(), paramsBuilder));

Ok got it now. Earlier I thought 0 is key. But you are proposing that key should be Span.tags and value should be say span.kind.

For a map, there are two arguments one is key and the other is value - e.g x['key'] = 'value'
So, here Span.tags is a map, the span.kind is a key arg.
In the case of CONTAINS_KEY and NOT_CONTAINS_KEY, it will have only one key arg. But, in the case of CONTAINS_KEY_VALUE, it will have both key and value args, and so value arg is optional in structure.
e.g Span.tags['span.kind'] = 'client', for this key will be span.kind and value will be client

Should I make this change then ?

Yes, we can do that.

Alternatively, we can wait then drop this whole thing. Assuming we deprecate CONTAINS_KEY_VALUE and remove the array around the contains_key arg, this whole function can just go away I think?

Yes. We can simplify that and move inline maybe.

…ontains_key_support

sarthak77 · 2021-12-16T08:19:30Z

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java

-  private LiteralConstant[] convertExpressionToMapLiterals(Expression expression) {
-    LiteralConstant[] literals = new LiteralConstant[2];
+  private List<LiteralConstant> convertExpressionToMapLiterals(Expression expression) {
+    List<String> literals = new ArrayList<>(List.of("", ""));
    if (expression.getValueCase() == LITERAL) {


Also, do we throw an exception here if rhs is not literal ? As using the below design, we always expect the key. Also, we are always calling this method from such a case so it may prevent any error if some new case is added.

@Value class MapKeyValueArguments { LiteralConstant key; Optional<LiteralConstant> value; }

CC @kotharironak

In the existing main code, we are defaulting to empty string "" if the case is not LITERAL. Here as well default to "" string LiteralConstant.

I thought optional was to deal with this default ""

Optional value means that the map's value argument is not mandatory. Refer to this - #125 (comment)

So, in a way, we can also extend as you suggested that we expect the key argument should be present. if it's not there, we can throw the exception. And for the value, we can default to ""

Does the below make sense?
spanTags contains_key ''

aaron-steinfeld · 2021-12-16T16:55:38Z

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java

+          List<LiteralConstant> kvp = convertExpressionToMapLiterals(filter.getRhs());
+          builder.append(convertExpressionToMapKeyColumn(filter.getLhs()));
+          builder.append(" != ");
+          builder.append(convertLiteralToString(kvp.get(MAP_KEY_INDEX), paramsBuilder));


CONTAINS_KEY and NOT_CONTAINS_KEY can really just have the same logic - assuming we translate the operators correctly in convertOperatorToString (they're not currently, have a separate comment on that), the only difference between these and the default case is the conversion of the LHS (using convertExpressionToMapKeyColumn instead of convertExpressionToString)

aaron-steinfeld · 2021-12-16T16:58:13Z

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java

@@ -250,6 +257,7 @@ private String convertOperatorToString(Operator operator) {
        return "<=";
      case LIKE:
        return REGEX_OPERATOR;
+      case NOT_CONTAINS_KEY:


These cases aren't right. It seems like they just got parked here so as not to throw since they're not used in the existing code and this is called prematurely. NOT_CONTAINS_KEY becomes != and CONTAINS_KEY becomes = (the only difference being that we change the col translation to add on __keys). CONTAINS_KEYVALUE becomes multiple expressions, so could remain until removed.

aaron-steinfeld · 2021-12-17T03:47:09Z

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java

-        }
+      if (value.getValue().getValueType() == ValueType.STRING_ARRAY
+          && value.getValue().getStringArrayCount() == 2) {
+        literals.set(0, value.getValue().getStringArray(0));


nit: if we're going to keep it unstructured and use a list, might as well use the same index constants to write this as we use to read it.

Didn't get you on this.

We read these as via .get(MAP_KEY_INDEX) etc., but we write them with plain literals that need to match those constants. Here, we should be using

literals.set(MAP_KEY_INDEX, value.getValue().getStringArray(MAP_KEY_INDEX));

Or better yet, using no constants and just use an object like we do in most java code as discussed earlier.

All relatively moot though as long as this function is slated for deletion in the near term.

github-actions · 2021-12-17T03:54:06Z

Unit Test Results

  30 files ±0   30 suites ±0 9s ⏱️ ±0s
174 tests +3 174 ✔️ +3 0 💤 ±0 0 ❌ ±0

Results for commit 85bcb31. ± Comparison against base commit 392d36b.

added support for contains key

1a5901c

sarthak77 requested review from aaron-steinfeld, a team, kotharironak and findingrish December 15, 2021 04:08

This comment has been minimized.

Sign in to view

added unit test

6df2378

sarthak77 changed the title ~~Fix : Add support for contains key operator~~ Fix : Add support for contains key and like operator Dec 15, 2021

This comment has been minimized.

Sign in to view

findingrish reviewed Dec 15, 2021

View reviewed changes

...l/src/main/java/org/hypertrace/core/query/service/pinot/QueryRequestToPinotSQLConverter.java Outdated Show resolved Hide resolved

aaron-steinfeld reviewed Dec 15, 2021

View reviewed changes

Sarthak Singhal added 2 commits December 16, 2021 09:08

nit

ab28671

Merge branch 'main' of github.com:hypertrace/query-service into fix_c…

f768fed

…ontains_key_support

This comment has been minimized.

Sign in to view

added support for not_contains_key operator

e8959b7

sarthak77 requested review from findingrish and aaron-steinfeld December 16, 2021 03:55

This comment has been minimized.

Sign in to view

sarthak77 commented Dec 16, 2021

View reviewed changes

aaron-steinfeld reviewed Dec 16, 2021

View reviewed changes

resolved pr comments

de8f2ce

sarthak77 requested a review from aaron-steinfeld December 17, 2021 03:32

This comment has been minimized.

Sign in to view

aaron-steinfeld approved these changes Dec 17, 2021

View reviewed changes

sarthak77 merged commit 85bcb31 into main Dec 17, 2021

sarthak77 deleted the fix_contains_key_support branch December 17, 2021 03:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix : Add support for contains key and like operator #125

Fix : Add support for contains key and like operator #125

sarthak77 commented Dec 15, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

aaron-steinfeld Dec 15, 2021

sarthak77 Dec 16, 2021

sarthak77 Dec 17, 2021

aaron-steinfeld Dec 15, 2021

findingrish Dec 15, 2021

aaron-steinfeld Dec 15, 2021

sarthak77 Dec 16, 2021

kotharironak Dec 16, 2021

kotharironak Dec 16, 2021

kotharironak Dec 16, 2021 •

edited

Loading

aaron-steinfeld Dec 16, 2021

sarthak77 Dec 16, 2021

sarthak77 Dec 17, 2021

This comment has been minimized.

This comment has been minimized.

sarthak77 Dec 16, 2021

sarthak77 Dec 16, 2021

kotharironak Dec 16, 2021

sarthak77 Dec 16, 2021

kotharironak Dec 16, 2021

kotharironak Dec 16, 2021 •

edited

Loading

aaron-steinfeld Dec 16, 2021 •

edited

Loading

sarthak77 Dec 17, 2021

aaron-steinfeld Dec 16, 2021

sarthak77 Dec 17, 2021

This comment has been minimized.

aaron-steinfeld Dec 17, 2021

sarthak77 Dec 17, 2021

aaron-steinfeld Dec 17, 2021

sarthak77 Dec 17, 2021

github-actions bot commented Dec 17, 2021

Fix : Add support for contains key and like operator #125

Fix : Add support for contains key and like operator #125

Conversation

sarthak77 commented Dec 15, 2021 • edited Loading

codecov bot commented Dec 15, 2021 • edited Loading

Codecov Report

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotharironak Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotharironak Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

aaron-steinfeld Dec 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2021

Unit Test Results

sarthak77 commented Dec 15, 2021 •

edited

Loading

codecov bot commented Dec 15, 2021 •

edited

Loading

kotharironak Dec 16, 2021 •

edited

Loading

kotharironak Dec 16, 2021 •

edited

Loading

aaron-steinfeld Dec 16, 2021 •

edited

Loading