Skip to content

Conversation

@atris
Copy link
Contributor

@atris atris commented Jun 8, 2022

This PR implements a real time FST index, allowing fields containing tokenized documents to be directly stored in the FST index and queried in real time, with no flush requirement.

This can be enabled by specifying the following property in the properties section of the FST index, being created on a real time table:
"properties":[{"fstType":"native"}]

@codecov-commenter
Copy link

codecov-commenter commented Jun 8, 2022

Codecov Report

Attention: Patch coverage is 61.76471% with 13 lines in your changes missing coverage. Please review.

Project coverage is 69.74%. Comparing base (dbfe39e) to head (332e55f).
Report is 3282 commits behind head on master.

Files with missing lines Patch % Lines
...local/indexsegment/mutable/MutableSegmentImpl.java 20.00% 10 Missing and 2 partials ⚠️
...time/impl/invertedindex/NativeMutableFSTIndex.java 94.73% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #8861      +/-   ##
============================================
+ Coverage     69.70%   69.74%   +0.04%     
- Complexity     4668     4990     +322     
============================================
  Files          1741     1804      +63     
  Lines         91521    93883    +2362     
  Branches      13691    13981     +290     
============================================
+ Hits          63793    65478    +1685     
- Misses        23289    23869     +580     
- Partials       4439     4536      +97     
Flag Coverage Δ
integration1 26.35% <0.00%> (-0.54%) ⬇️
integration2 24.99% <0.00%> (-0.66%) ⬇️
unittests1 66.44% <61.76%> (+0.24%) ⬆️
unittests2 15.51% <0.00%> (+1.39%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Jackie-Jiang Jackie-Jiang added feature release-notes Referenced by PRs that need attention when compiling the next release notes labels Jun 8, 2022
Copy link
Contributor

@Jackie-Jiang Jackie-Jiang left a comment

Choose a reason for hiding this comment

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

Good job! Only minor comments

Please add some description to the PR, and add a release-note section

public class NativeMutableFSTIndex implements MutableTextIndex {
private final String _column;
private final MutableFST _fst;
private int _dictId;
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor)
Move this after the final variables for readability


@Override
public MutableRoaringBitmap getDocIds(String searchQuery) {
return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's throw an unsupported exception with some error message for debugging purpose

invertedIndexColumns.contains(column) ? indexProvider.newInvertedIndex(context.forInvertedIndex()) : null;

MutableTextIndex fstIndex = null;
//FST Index
Copy link
Contributor

Choose a reason for hiding this comment

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

(nit) Add a space after the //

Copy link
Contributor

Choose a reason for hiding this comment

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

We can remove the TODO on line 246


if (results != null) {
for (int result : results) {
assertEquals(resultMap.contains(result), true);
Copy link
Contributor

Choose a reason for hiding this comment

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

(minor) assertTrue

MutableTextIndex fstIndex = null;
//FST Index
if (fstIndexColumns.contains(column)) {
if (_fieldConfigList != null) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: you can combine these nested if statement into one like:
if (_fieldConfigList != null && fstIndexColumns.contains(column))


nativeQuery = ".*ed";
resultList = Arrays.asList(6);
testSelectionResults(nativeQuery, 1, resultList);
Copy link
Member

Choose a reason for hiding this comment

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

Add more tests like .*m.*

@atris atris merged commit 6984719 into apache:master Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature release-notes Referenced by PRs that need attention when compiling the next release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants