expect regex extracted tokens in database bloom filters #103

elliVM · 2024-10-22T05:10:17Z

Use regex to extract tokens from search term. Use tokens to select joined tables and generated bloommatch conditions.
Move bloom filter related classes to bloomfilter package
DatabaseTables interface
Tokenizable interface and decorator TokensAsStrings
CategoryTable interface refactored to only have create() method
Renaming of classes and variables and refactoring to improve code clarity

51-code

LGTM

eemhu · 2024-10-22T07:36:34Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/BloomFilterFromRecord.java

+                            "Null field while creating bloom filter expected <{}>, fpp <{}>, pattern <{}>, search term <{}>",
+                            expected, fpp, pattern, searchTerm
+                    );
+            throw new RuntimeException("Object field was null");


this exception message could be a bit clearer

Clarified the exception messages, added tests for excetpions, removed use of .longValue() method in constructor which would lead to NPE.

eemhu · 2024-10-22T08:20:04Z

src/main/java/com/teragrep/pth_06/planner/walker/conditions/IndexStatementCondition.java

@@ -78,7 +78,7 @@ public IndexStatementCondition(String value, ConditionConfig config, Condition c

    public Condition condition() {
        if (!config.bloomEnabled()) {
-            LOGGER.debug("Indexstatement reached with bloom disabled");
+            LOGGER.warn("Indexstatement reached with bloom disabled");


would this be more suitable to be a debug log?

yes that is better I lowered to debug

eemhu · 2024-10-22T08:21:00Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/TokenizedValue.java


    public TokenizedValue(String value) {
+        this(
+                value,
+                new HashSet<>(new Tokenizer(32).tokenize(new ByteArrayInputStream(value.getBytes(StandardCharsets.UTF_8))))


not 100% sure if calling tokenizer in constructor is optimal, perhaps it should only be done when tokens are required?

Refactored that tokenizer is called only when tokens are needed.

eemhu · 2024-10-22T08:45:03Z

src/test/java/com/teragrep/pth_06/planner/bloomfilter/BloomFilterFromRecordTest.java

+        Table<?> target = DSL.table(DSL.name("target"));
+        String searchTerm = "Pattern";
+        BloomFilterFromRecord filter = new BloomFilterFromRecord(dynamicRecord, target, searchTerm);
+        Assertions.assertDoesNotThrow(filter::bytes);


no assertions for bytes() result?

Removed since test was redundant, bytes() method is tested on the other tests.

eemhu · 2024-10-22T08:47:41Z

src/test/java/com/teragrep/pth_06/planner/bloomfilter/RegexExtractedValueTest.java

+        RegexExtractedValue regexValue = new RegexExtractedValue(value, regex);
+        Set<String> tokens = regexValue.tokens();
+        Assertions.assertEquals(2, tokens.size());
+        Assertions.assertTrue(tokens.contains("(important)") && tokens.contains("(very important)"));


separate into two assertions for clarity

Separated into two assertions.

…rify exception messages. Add tests for exceptions.

…isabled

kortemik · 2024-10-22T11:08:50Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/BloomFilterFromRecord.java

+    public byte[] bytes() {
+        final BloomFilter filter = create();
+        final ByteArrayOutputStream filterBAOS = new ByteArrayOutputStream();
+        try {


why not try-with-resources?

refactored to use try-with-resources

src/main/java/com/teragrep/pth_06/planner/bloomfilter/TableFilterTypesFromMetadata.java

kortemik · 2024-10-22T12:08:54Z

...in/java/com/teragrep/pth_06/planner/bloomfilter/FilterFromRecordToCategoryTableConsumer.java

- * Filter types of a table that can be inserted into the tables category table
- */
-public final class TableFilters {
+public final class FilterFromRecordToCategoryTableConsumer implements Consumer<Record> {


please use object way instead of functional way for producing an iterator meaning for loop instead of a consumer

refactored to use for loop

…ntext equality

kortemik · 2024-10-22T13:49:02Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/RegexExtractedValue.java

+
+public final class RegexExtractedValue {
+
+    private final Matcher matcher;


matcher is stateful https://docs.oracle.com/javase/8/docs/api/java/util/regex/Matcher.html#find-- this objects is therefore mutable and mutability is a no-go.

Replaced Matcher with Pattern that is stateless and immutable.

kortemik · 2024-10-22T13:54:51Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/BloomFilterFromRecord.java

+        }
+        final BloomFilter filter = BloomFilter.create(expected.longValue(), fpp);
+        // if no pattern use tokenized value (currently BLOOMDB.FILTERTYPE.PATTERN is NOT NULL)
+        if (pattern == null) {


object is configurable

Refactored object to be not configurable

src/main/java/com/teragrep/pth_06/planner/bloomfilter/TokenizedValue.java

kortemik · 2024-10-22T14:08:56Z

src/test/java/com/teragrep/pth_06/planner/walker/conditions/PatternMatchConditionTest.java

@@ -64,18 +65,6 @@ void testSingleToken() {
        Assertions.assertEquals(e, condition.toString());
    }

-    @Test


removing tests is not a good idea. please comment why tests were removed

Previously depending on the input for the PatternMatchCondition(input) the result of condition() varied depending on the number of tokens generated from input. After change to support regex extracted tokens the input is no longer tokenized, making the removed test a duplicate of the first test.

src/test/java/com/teragrep/pth_06/planner/bloomfilter/TableFiltersTest.java

…d and make it unconfigurable, make matcher immutable

kortemik · 2024-10-24T09:48:34Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/SearchTermBloomFilter.java

+                    "Trying to insert empty filter, pattern match joined table should always have tokens"
+            );
+        }
+        final BloomFilter filter = BloomFilter.create(1000, 0.01);


1000 and 0.01 why not expected and fpp?

…pplied.

51-code

Added some request changes. Also some classes are missing equals or hashcode methods, at least CategoryTableImpl and CategoryTableWithFilters.

51-code · 2024-10-29T11:10:28Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/SearchTermBloomFilter.java

+            return filterBAOS.toByteArray();
+        }
+        catch (IOException e) {
+            throw new UncheckedIOException(new IOException("Error writing filter bytes: " + e.getMessage()));


Instead of creating a new IOException here, the existing one could be passed to the UncheckedIOException using the other constructor: UncheckedIOException(String message, IOException cause)

refactored to use constructor

51-code · 2024-10-29T11:26:11Z

src/main/java/com/teragrep/pth_06/planner/bloomfilter/TableFilters.java

+                    DSL.val(record.getValue(BLOOMDB.FILTERTYPE.ID), ULong.class),
+                    DSL.val(filter.bytes(), byte[].class)
+            };
+            ctx.insertInto(categoryTable).columns(insertFields).values(valueFields).execute();


I looked into the docs, I wonder if it's possible to return a Query from this function (probably what the values() function returns before the execute function is called. Currently it feels wrong that the filters are executed here, which makes this kind of a utility class for the CategoryTable. If it would return the Query (in my mind this is the tablefilter itself) then it could be executed in the CategoryTableWithFilters object. Does this make any sense?

TableFilters now returns a SafeBatch class that wraps a jooq.Batch class, CategoryTableWithFilters will execute the SafeBatch.

...a/com/teragrep/pth_06/planner/walker/conditions/RegexLikeFiltertypePatternConditionTest.java

…ition

…vadoc and exception messages

src/main/java/com/teragrep/pth_06/planner/bloomfilter/ConditionMatchBloomDBTables.java

...est/java/com/teragrep/pth_06/planner/bloomfilter/TableFilterTypesFromMetadataResultTest.java

src/main/java/com/teragrep/pth_06/planner/bloomfilter/SafeBatch.java

… remove sout

pom.xml

…d, remove logcaptor dependency

src/test/java/com/teragrep/pth_06/planner/bloomfilter/TokensAsStringsTest.java

src/test/java/com/teragrep/pth_06/planner/bloomfilter/TableFiltersTest.java

kortemik · 2024-11-05T15:50:52Z

@eemhu @51-code please re-review

eemhu

changes lgtm

51-code

LGTM

expect regex extracted tokens in database bloom filters

b9d2731

elliVM self-assigned this Oct 22, 2024

elliVM added 2 commits October 22, 2024 08:34

add RegexExtractedValueTest

465cf83

remove unnecessary test

7d4559d

elliVM requested a review from 51-code October 22, 2024 07:04

elliVM marked this pull request as ready for review October 22, 2024 07:05

51-code approved these changes Oct 22, 2024

View reviewed changes

elliVM requested a review from eemhu October 22, 2024 07:21

eemhu reviewed Oct 22, 2024

View reviewed changes

elliVM added 4 commits October 22, 2024 12:26

TokenizedValue: call tokenizer only when needed, clean up tests

945e839

clear up exception message in BloomFilterFromRecord

2a8d97c

BloomFilterFromRecord: remove ULong.longValue() from constructor, cla…

b3ac348

…rify exception messages. Add tests for exceptions.

set logger level to debug when indexstatement is reached with bloom d…

0d72406

…isabled

elliVM requested a review from eemhu October 22, 2024 09:45

kortemik requested changes Oct 22, 2024

View reviewed changes

elliVM added 2 commits October 22, 2024 15:12

remove consumer class and use for loop in TableFilters

a682789

use try with resources and add comments on equals methods about DSLCo…

b354614

…ntext equality

kortemik requested changes Oct 22, 2024

View reviewed changes

kortemik reviewed Oct 22, 2024

View reviewed changes

add Tokenizable interface and decorators, rename BloomFilterFromRecor…

5f907c8

…d and make it unconfigurable, make matcher immutable

elliVM requested a review from kortemik October 23, 2024 10:14

elliVM added 2 commits October 23, 2024 13:40

add missing assertion to test

0bf7410

move method after constructors

31e0bf9

kortemik requested changes Oct 24, 2024

View reviewed changes

fix hard coded filter size and fix testing that different sizes are a…

f370356

…pplied.

51-code requested changes Oct 29, 2024

View reviewed changes

elliVM added 3 commits October 29, 2024 14:35

use UncheckedIOException constructor

aefb66a

TableFilters returns a batch that CategoryTableWithFilters executes

73efeb9

add test for SafeBatch

441e40b

elliVM requested a review from 51-code October 29, 2024 13:12

elliVM commented Oct 29, 2024

View reviewed changes

...a/com/teragrep/pth_06/planner/walker/conditions/RegexLikeFiltertypePatternConditionTest.java Show resolved Hide resolved

elliVM added 4 commits October 29, 2024 15:47

update comments and clean up code, add constructors for RegexLikeCond…

4844ebd

…ition

use qualified names update tests

145fcb6

more descriptive naming of methods and variables, update comments, ja…

40c2165

…vadoc and exception messages

apply spotless

6513861

eemhu reviewed Oct 31, 2024

View reviewed changes

src/main/java/com/teragrep/pth_06/planner/bloomfilter/ConditionMatchBloomDBTables.java Show resolved Hide resolved

eemhu reviewed Oct 31, 2024

View reviewed changes

...est/java/com/teragrep/pth_06/planner/bloomfilter/TableFilterTypesFromMetadataResultTest.java Outdated Show resolved Hide resolved

eemhu reviewed Oct 31, 2024

View reviewed changes

src/main/java/com/teragrep/pth_06/planner/bloomfilter/SafeBatch.java Outdated Show resolved Hide resolved

elliVM added 2 commits October 31, 2024 11:04

add missing hashCode() methods

adc7faf

don't wrap jooq.Batch object and execute in CategoryTableWithFilters,…

2c01634

… remove sout

elliVM requested review from eemhu and kortemik October 31, 2024 12:21

kortemik reviewed Nov 1, 2024

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

throw exception if search term filter tokens size larger than expecte…

34efb98

…d, remove logcaptor dependency

kortemik requested changes Nov 1, 2024

View reviewed changes

improve TableFiltersTest and TokensAsStringsTest

7412ec2

elliVM requested a review from kortemik November 1, 2024 13:29

kortemik approved these changes Nov 5, 2024

View reviewed changes

eemhu approved these changes Nov 6, 2024

View reviewed changes

51-code approved these changes Nov 6, 2024

View reviewed changes

allow search term filter tokens to be larger than expected tokens

7dea6f7

elliVM requested a review from kortemik November 11, 2024 08:57

kortemik approved these changes Nov 11, 2024

View reviewed changes

kortemik merged commit 51c80ca into teragrep:main Nov 11, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expect regex extracted tokens in database bloom filters #103

expect regex extracted tokens in database bloom filters #103

elliVM commented Oct 22, 2024 •

edited

Loading

51-code left a comment

eemhu Oct 22, 2024

elliVM Oct 22, 2024

eemhu Oct 22, 2024

elliVM Oct 22, 2024

eemhu Oct 22, 2024

elliVM Oct 22, 2024

eemhu Oct 22, 2024

elliVM Oct 22, 2024

eemhu Oct 22, 2024

elliVM Oct 22, 2024

kortemik Oct 22, 2024

elliVM Oct 22, 2024

kortemik Oct 22, 2024

elliVM Oct 22, 2024

kortemik Oct 22, 2024

elliVM Oct 23, 2024

kortemik Oct 22, 2024

elliVM Oct 23, 2024

kortemik Oct 22, 2024

elliVM Oct 23, 2024

kortemik Oct 24, 2024

51-code left a comment

51-code Oct 29, 2024

elliVM Oct 29, 2024

51-code Oct 29, 2024

elliVM Oct 29, 2024

kortemik commented Nov 5, 2024

eemhu left a comment

51-code left a comment


		public final class RegexExtractedValue {

		private final Matcher matcher;

expect regex extracted tokens in database bloom filters #103

expect regex extracted tokens in database bloom filters #103

Conversation

elliVM commented Oct 22, 2024 • edited Loading

51-code left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

51-code left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kortemik commented Nov 5, 2024

eemhu left a comment

Choose a reason for hiding this comment

51-code left a comment

Choose a reason for hiding this comment

elliVM commented Oct 22, 2024 •

edited

Loading