feat: add CONTAINS_KEY_LIKE operator#145
Conversation
| convertExpressionToString(filter.getLhs(), paramsBuilder, executionContext)); | ||
| if (isMapColumnName(filter.getLhs())) { | ||
| // supports Like operator on map key column | ||
| builder.append(convertExpressionToMapKeyColumn(filter.getLhs())); |
There was a problem hiding this comment.
On second thought, this does a regex search only on the field's keys and not on its values. Should we do a regex search on the field's values as well here, or should we introduce new operators like - KEY_LIKE and VALUE_LIKE and leave the LIKE operator unchanged?
There was a problem hiding this comment.
We already support operations on values via attribute expressions (e.g. tags.some_key LIKE myValue.*). We don't really want to introduce new operators, but the only potentially useful scenariosI could think of that we would be missing would be extensions of the CONTAINS_KEY operator - e.g. NOT_CONTAINS_KEY and CONTAINS_KEY_LIKE.
A contains_value is an unsupported scenario too, but I'm not really sure that's useful (or performant enough) to warrant implementation.
There was a problem hiding this comment.
@GurtejSohi CONTAINS_KEY_LIKE is what i think we need
There was a problem hiding this comment.
Removed these changes and added a new operator: CONTAINS_KEY_LIKE
There was a problem hiding this comment.
We don't really want to introduce new operators, but the only potentially useful scenariosI could think of that we would be missing would be extensions of the CONTAINS_KEY operator - e.g. NOT_CONTAINS_KEY and CONTAINS_KEY_LIKE.
Actually, we already have NOT_CONTAINS_KEY operator.
There was a problem hiding this comment.
Actually, we already have NOT_CONTAINS_KEY operator.
I believe we implemented at QS but haven't exposed it up to the platform API layer.
Codecov Report
@@ Coverage Diff @@
## main #145 +/- ##
============================================
+ Coverage 82.13% 82.19% +0.05%
- Complexity 633 634 +1
============================================
Files 66 66
Lines 2385 2393 +8
Branches 244 244
============================================
+ Hits 1959 1967 +8
Misses 329 329
Partials 97 97
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
GurtejSohi
changed the title
aaron-steinfeld
left a comment
aaron-steinfeld
left a comment
There was a problem hiding this comment.
LGTM. Need to get integration tests fixed first, which is a pinot upgrade issue (ran into the same thing on my PR #143 (comment) - hoping to return to it later today unless you get a chance first.
@aaron-steinfeld I was looking into this today. As pointed by the logs here, this check is failing. While debugging, observed that this map of kafka consumer group offsets is empty, which implies that pinot is not consuming from the topics written to by view-generator (maybe some change in ingestion config key in the new version of pinot). Tried looking into it, but couldn't find the reason. |
Yes, I spent a few hours on this today. There's a lot going on, but the customer pinot service manager build wasn't updated to work with the current pinot version that it unpacks. One issue is its logging, but there seems to be a change in the consumer group behavior too. That said, it does actually ingest, it's just this check that's broken. Hoping @laxman-traceable or someone with more pinot and zookeeper familiarity can assist here. |
|
It can’t find that Will fix the tests accordingly. |
|
Thanks to @laxmanchekka and @kotharironak integration tests have been fixed as part of my PR and merged. please rebase to pick it up. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
4 participants
Description
This PR adds
CONTAINS_KEY_LIKEoperator for doing a regex based search on map fields (on map key column).Testing
Added unit test.
Checklist: