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

fix: adds values filter for map column #190

Merged
merged 3 commits into from
Jul 15, 2023
Merged

Conversation

kotharironak
Copy link
Contributor

@kotharironak kotharironak commented Jul 14, 2023

Description

This pull request introduces an additional value filter for map columns, aiming to enhance performance by utilizing the underlying inverted index on the __keys and __values columns of the Pinot implementation.

This optimisation is specifically applied to queries like mapValue(tags__keys, 'flags', tags__values) = '0'.

This approach is the same as the one followed in the deprecated CONTAINS_KEYVALUE.
Ref:

Testing

Modified the existing test case.

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 Jul 14, 2023

Codecov Report

Merging #190 (a416254) into main (b89ebf4) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #190      +/-   ##
============================================
+ Coverage     78.83%   78.90%   +0.06%     
- Complexity      902      906       +4     
============================================
  Files            80       80              
  Lines          3661     3673      +12     
  Branches        420      421       +1     
============================================
+ Hits           2886     2898      +12     
  Misses          599      599              
  Partials        176      176              
Flag Coverage Δ
integration 78.90% <100.00%> (+0.06%) ⬆️
unit 73.76% <100.00%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
...service/pinot/QueryRequestToPinotSQLConverter.java 92.06% <100.00%> (+0.34%) ⬆️

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

@github-actions

This comment has been minimized.

Optional<String> valueFilter =
getValuesFilterForMapColumn(
filter.getLhs(), rhs, filter.getOperator(), paramsBuilder, executionContext);
valueFilter.stream().forEach(str -> builder.append(str));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
valueFilter.stream().forEach(str -> builder.append(str));
valueFilter.ifPresent(builder::append);

since there is no collection within Optional

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.

String valCol = convertExpressionToMapValueColumn(lhs);
return Optional.of(
String.format(
"%s %s %s AND ",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: we can take AND outside this function, so that it can be reused elsewhere as a terminal filter condition (i.e. when there is no other filter after it).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, ok.

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 5636046 into main Jul 15, 2023
@kotharironak kotharironak deleted the apply-value-filter branch July 15, 2023 04:55
@github-actions
Copy link

Unit Test Results

  39 files  ±0    39 suites  ±0   10s ⏱️ ±0s
263 tests ±0  263 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit 5636046. ± Comparison against base commit b89ebf4.

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.

3 participants