Skip to content
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

feat: add support for utilising text indexes if exits for like operator in pinot based handlers #186

Merged
merged 6 commits into from
Jun 13, 2023

Conversation

kotharironak
Copy link
Contributor

@kotharironak kotharironak commented Jun 13, 2023

Description

This PR add support for utilising text indexes for LIKE operator for pinot based request handler if the index for those column is enabled. So, the string column with index will translated to

  • text_match(column, /<value>/)

Testing

  • Added the request conversation test.

Checklist:

  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • Any dependent changes have been merged and published in downstream modules

@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #186 (fe030cc) into main (a9aaeba) will increase coverage by 0.11%.
The diff coverage is 96.15%.

@@             Coverage Diff              @@
##               main     #186      +/-   ##
============================================
+ Coverage     78.68%   78.80%   +0.11%     
- Complexity      887      898      +11     
============================================
  Files            80       80              
  Lines          3626     3651      +25     
  Branches        413      419       +6     
============================================
+ Hits           2853     2877      +24     
  Misses          598      598              
- Partials        175      176       +1     
Flag Coverage Δ
integration 78.80% <96.15%> (+0.11%) ⬆️
unit 73.63% <96.15%> (+0.15%) ⬆️

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

Impacted Files Coverage Δ
...service/pinot/QueryRequestToPinotSQLConverter.java 91.72% <93.33%> (+0.05%) ⬆️
...race/core/query/service/pinot/PinotColumnSpec.java 100.00% <100.00%> (ø)
...trace/core/query/service/pinot/ViewDefinition.java 91.46% <100.00%> (+0.79%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

return REGEX_OPERATOR;
}

private Expression prefixValueForLikeOperator(String likeOperatorStr, Expression rhsExpression) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For TEXT_MATCH regex query needs string to be prefixed and suffixed with '/'.

@kotharironak kotharironak changed the title feat: add support for handling text indexes in pinot feat: add support for utilising text indexes if exits for like operator in pinot based handlers Jun 13, 2023
@github-actions

This comment has been minimized.

@kotharironak kotharironak marked this pull request as ready for review June 13, 2023 14:49
@github-actions

This comment has been minimized.

Expression rhs =
handleValueConversionForLiteralExpression(filter.getLhs(), filter.getRhs());
rhs = postProcessValueConversionForLikeOperator(operator, rhs);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: rather than taking multiple steps to calculate the RHS in the top level, could you do something like

          operator = handleLikeOperatorConversion(filter.getLhs());
          Expression rhs =
              handleValueConversionForLikeArgument(filter.getLhs(), filter.getRhs());

and then have handleValueConversionForLikeArgument do the delegation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

private Expression postProcessValueConversionForLikeOperator(
String likeOperatorStr, Expression rhsExpression) {
if (likeOperatorStr.equals(TEXT_MATCH_OPERATOR)) {
String strValue = "/" + rhsExpression.getLiteral().getValue().getString() + "/";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this could be simplified a bit by just starting from the input.

Builder builder = rhsExpression.toBuilder()
 if (condition) {
   builder.getLiteralBuilder().getValueBuilder().setString(...);
 }
return builder.build();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@kotharironak kotharironak merged commit 6b3081e into main Jun 13, 2023
@kotharironak kotharironak deleted the text-match-index branch June 13, 2023 17:14
@github-actions
Copy link

Unit Test Results

  39 files  ±0    39 suites  ±0   8s ⏱️ -4s
262 tests +2  262 ✔️ +2  0 💤 ±0  0 ❌ ±0 

Results for commit 6b3081e. ± Comparison against base commit a9aaeba.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants