-
Notifications
You must be signed in to change notification settings - Fork 6
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
Pattern acceleration support #65
Conversation
…atches, refactor ConditionWalkers and improve tests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the objects are not immutable, is it fixable or perhaps out of the scope of this PR?
All new classes now have only final values and public methods always return the same value after instantiation (weakly immutable). I included some refactoring of old classes but I think the project would require a larger refactoring to be more object-oriented. Made project issues regarding refactoring |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me!
import static com.teragrep.pth_06.jooq.generated.bloomdb.Bloomdb.BLOOMDB; | ||
import static org.jooq.impl.SQLDataType.*; | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please write detailed javadoc about the logic behind the table structure and the query pattern. It is not very obvious to decode the schema and the query from the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
39adb13 Added better javadoc for BloomFilterTempTable in an effort to clear up the logic.
.leftJoin(BLOOMDB.FILTERTYPE) | ||
.on(BLOOMDB.FILTERTYPE.ID.eq((Field<ULong>) t.field("filter_type_id"))) | ||
.where(finalPatternCondition) | ||
.limit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why limit 1 is present? this limits the search token to make only one of the possible bloomfilters as a candidate instead of having all as possible candidates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please comment the code to indicate the purpose
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All tables in bloomdb are checked in turn by the filterTables() function, limit is just for the single table to avoid loading all of it's rows to memory, since it makes the filter if it has even just 1 matching row.
I will add the comment to clear the purpose
Not knowledgeable enough about this functionality to give explicit approval
.leftJoin(BLOOMDB.FILTERTYPE) | ||
.on(BLOOMDB.FILTERTYPE.ID.eq((Field<ULong>) t.field("filter_type_id"))) | ||
.where(finalPatternCondition) | ||
.limit(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please comment the code to indicate the purpose
private final ConditionConfig config; | ||
private final Set<Table<?>> tableSet; | ||
private final long bloomTermId; | ||
private final List<Condition> conditionCache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a cache is being used?
private final Table<Record> tableName; | ||
private final long bloomTermId; | ||
private final Set<Token> tokenSet; | ||
private final List<Condition> cache; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why a cache is being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to limit the SQL operations if the Condition is already generated. Should I refactor them to work without caches?
} | ||
|
||
@Test | ||
void fromStringIntendTest() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing existing test will not do, please revert the changes.
Refactoring of old code split into another PR |
Moving PR to another branch |
Pattern acceleration