use ngram in TrieSearch as it performs very similarly but at much low… #3216

awildturtok · 2023-11-15T15:26:31Z

…er memory footprint

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

awildturtok · 2024-01-18T09:16:03Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

-							  });
+			for (final List<KeywordIndex> hits : trie.prefixMap(keyword).values())
+				updateWeights(keyword, hits, itemWeights);
+			if (keyword.length() > ngramLength) {


Hier ist deine Formatierung off, ich muss dir noch meine config schicken.

awildturtok · 2024-01-18T09:19:13Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

 		);
 	}

-	private void doPut(String kw, T item) {
+	private void doPut(NgramIndex ni) {
+		// wouldn't it suffice to check once in addItem()?


sowas bitte mit // TODO markieren, aber gute Frage. Vermutlich hast du recht.

awildturtok · 2024-01-18T09:21:37Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

 	}

 	public Iterator<T> iterator() {
 		// This is a very ugly workaround to not get eager evaluation (which happens when using flatMap and distinct on streams)
 		final Set<T> seen = new HashSet<>();

 		return Iterators.filter(
-				Iterators.concat(Iterators.transform(trie.values().iterator(), Collection::iterator)),
+				Iterators.concat(Iterators.transform(keywordItemsList.stream().map(ki -> ki.items).iterator(), Collection::iterator)),


Hier bitte kein stream benutzen, die ganze Methode sollte eigentlich stream-basiert sein, streams haben aber buggy verhalten was sich erst bei sehr großen Mengen äußert.

Das wäre dann ein Iterators.transform

awildturtok · 2024-01-18T09:22:27Z

backend/src/test/java/com/bakdata/conquery/util/search/QuickSearchTest.java

@@ -84,34 +84,33 @@ public void searchIdentities() {
 	public void testSuffixes() {
 		final TrieSearch<String> search = new TrieSearch<>(2, null);

-		assertThat(search.suffixes("baaacd"))
+		assertThat(search.ngramSplitStrings("baaacd", "item"))


"item" ist ein nicht eingegebenes Wort um zu testen, dass du es nicht rauskriegst?

awildturtok · 2024-01-18T09:27:22Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

 	}

-	Stream<String> suffixes(String word) {
+	private Stream<NgramIndex> ngramSplit(String word, T item) {


Du mischt hier Zustand und Computation, bitte trennen. Das sieht mir eher nach Teilen von doPut aus?

awildturtok

Sieht super aus! Danke

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

awildturtok · 2024-01-24T10:38:34Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

 	/**
 	 * Maps from keywords to associated items.
 	 */
-	private final PatriciaTrie<List<T>> trie = new PatriciaTrie<>();
+	private final PatriciaTrie<List<Integer>> trie = new PatriciaTrie<>();


Wenn du den Index nur als lookup nach keywordItemsList verwendest, dann einfach direkt das KeywordItems reinsetzen stattdessen. Java Objekte sind referenzen ;)

Alternativ IntList/IntArrayList verwenden.

awildturtok · 2024-01-24T10:39:27Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+			index = keywordItemsList.size() - 1;
+		}
+
+		ToTrieKeys(word).forEach(key -> doPut(key, index, item));


bitte normale for-loop verwenden.

awildturtok · 2024-01-24T10:39:54Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+		ToTrieKeys(word).forEach(key -> doPut(key, index, item));
+	}
+
+	private void doPut(String key, int index, T item) {


zwei mal doPut ist etwas ungeschickt, kannst du die bitte sauberer benennen?

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

awildturtok · 2024-01-24T10:41:19Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

 	}

 	public void logStats() {
+		// ToDo: meaning changed


was hat sich hier verändert?

(glaube die methode wird nicht gerufen)

Intellij meldet das TrieSearch::logStats und TrieSearch::listItems ungenutzt sind. Sollen diese Methoden entfernt werden?

ja können beide weg

backend/src/test/java/com/bakdata/conquery/integration/tests/FilterAutocompleteTest.java

backend/src/test/java/com/bakdata/conquery/util/search/QuickSearchTest.java

awildturtok · 2024-01-24T10:50:23Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+				}
+			}
+			else {


für sowas benutze ich gerne if(edge case) { ...; continue }

Aber ich glaube wir müssen das allein schon drin haben, weil das beim splitten entsthene kann und ggf auch itemWords kürzer als ngrams sein können

Co-authored-by: awildturtok <1553491+awildturtok@users.noreply.github.com>

awildturtok · 2024-01-29T11:38:32Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

 	}

 	public void logStats() {
+		// ToDo: meaning changed


ja können beide weg

awildturtok · 2024-01-29T11:39:43Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

@@ -221,7 +236,7 @@ public void shrinkToFit() {
 			return;
 		}

-		trie.replaceAll((key, values) -> values.stream().distinct().collect(Collectors.toList()));
+		trie.replaceAll((key, values) -> new ArrayList<>(values));


wissen wir hier schon, dass values distinct sind?

Nein, Ich weiß auch, dass die Values nicht distinct sind und habe .distinct() rausgenommen, weil das Verhalten vom TrieSearch sich ändert, je nachdem ob TrieSearch::shrinkToFit aufgerufen wurde.

Zudem führt das zum folgendem Ergebnis:

Expected :["Pants", "PantsPants", "Pantshop", "Sweatpants"] Actual :["Pants", "Pantshop", "PantsPants", "Sweatpants"]

Habe einen count Feld hinzugefügt, dass eine Art .distinct() durch Gruppierung und Summierung ermöglicht. So ist das Verhalten unabhängig davon ob TrieSearch::shrinkToFit aufgerufen wurde.

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

awildturtok · 2024-01-29T11:43:02Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+		}
+		ki.items.add(item);
+
+		for (final String key : (Iterable<String>) toTrieKeys(word)::iterator) {


sorry, dass das ein Stream ist hatte ich übersehen, da war die forEach dann angebracht.

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

Co-authored-by: awildturtok <1553491+awildturtok@users.noreply.github.com>

awildturtok · 2024-01-31T08:59:33Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+
+		for (final KeywordItemsCount<T> entry : hits) {
+			//			KeywordItems<T> ki = entry.keywordItems;
+			final double weight = Math.pow(weightWord(query, entry.word), entry.count);


Wenn ich das richtig verstehe ist das dafür da, dass wenn ein keyword öfters inserted wurde es höher priorisiert auszugeben?

…r creation

backend/src/main/java/com/bakdata/conquery/models/datasets/Column.java

...c/main/java/com/bakdata/conquery/models/datasets/concepts/filters/specific/SelectFilter.java

backend/src/main/java/com/bakdata/conquery/models/index/FrontendValueIndex.java

backend/src/main/java/com/bakdata/conquery/models/index/IndexService.java

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

thoniTUB · 2024-02-14T16:18:28Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+		// This barrier avoids work when shrinking and therefore unnecessary calls to entries::computeIfAbsent
+		final Set<String> barrier = new HashSet<>();


Ist der Lookup im particiatrie so aufwendig? und wenn ja, warum ist barrier eine locale Variable?

Das verhindert, dass ein item mehrfach zum gleichen ngram assoziiert wird. (wir sind hier in addItem)

Spart Arbeit beim Shrinken. Lokal wegen dem genannten Grund. Dieser Fall kann trotzdem eintreten, wenn ein Item nochmal mit Keywords eingefügt wird, die bereits existierende ganze Wörter oder ngrams beinhalten. Um das zu verhindern, braucht es eine kompliziertere Datenstruktur, die bis zum Shrinken behalten wird.

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

…ervice.java Co-authored-by: MT <12283268+thoniTUB@users.noreply.github.com>

…ndValueIndex.java Co-authored-by: MT <12283268+thoniTUB@users.noreply.github.com>

awildturtok · 2024-02-15T08:31:17Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+	/**
+	 * A lower weight implies more relevant words.
+	 */
+	private long weightWord(String query, String itemWord, boolean original) {


Ist die aktualisierte weightWord abgeleitet aus deinen Benchmarks?

Meine Überlegung ist, dass die Weight-Function möglichst simpel sein soll und dass bei einer kleineren oberen Grenze der Weight-Function, erst bei sehr vielen Hits (> 2^55) ein Overflow bei einem long entsteht. Dann habe ich das getestet und die Such-Ergebnisse waren gut.

awildturtok

Die Anpassungen im weighing würde ich gerne nachvollziehen wollen.

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

awildturtok · 2024-02-15T16:50:42Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+	 */
+	private long weightWord(String query, String itemWord, boolean original) {
+		// The weight function needs to be fast, as it is called frequently.
+		final long weight;


@thoniTUB Sebastian hat den Code jetzt so stark vereinfacht, dass man den fast komplett unrollen könnte, wäre das für dich einfacher?

if(exactMatch){ if(original) return 100; return 10; } if(original) return 4; return 2;

Die Zahlen sind auch gar nicht so wichtig wie man denken könnte. Solange relevante Ergebnisse kommen (was die ngrame garantieren) ist es gut genug.

awildturtok · 2024-02-15T16:54:11Z

Wegen mir kannst du das as-is mergen oder die kleinen Sachen noch anpassen und dann mergen.

Co-authored-by: awildturtok <1553491+awildturtok@users.noreply.github.com>

thoniTUB · 2024-02-22T09:26:33Z

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java

+		if (word.length() < ngramLength) {
+			return Stream.empty();
+		}


Sorry bisschen spät, ist mir gerade ins Auge gefallen.
Sollte es nicht

if (word.length() <= ngramLength) {

sein

du hast recht, ich fixe es

use ngram in TrieSearch as it performs very similarly but at much low…

97e9382

…er memory footprint

thoniTUB reviewed Nov 16, 2023

View reviewed changes

backend/src/main/java/com/bakdata/conquery/util/search/TrieSearch.java Outdated Show resolved Hide resolved

awildturtok assigned SebChmie Dec 19, 2023

SebChmie added 3 commits January 9, 2024 20:33

Fix ngram usage

36212e5

Fix indentation

ead4307

Simplify if case

7dfda54

awildturtok commented Jan 18, 2024

View reviewed changes

SebChmie added 2 commits January 23, 2024 14:53

Review changes

2a25b6a

Refactor code

8e1e56b

awildturtok commented Jan 24, 2024

View reviewed changes

SebChmie and others added 2 commits January 24, 2024 12:44

Apply suggestions from code review

44815a0

Co-authored-by: awildturtok <1553491+awildturtok@users.noreply.github.com>

Review changes

3be7d04

awildturtok commented Jan 29, 2024

View reviewed changes

SebChmie and others added 3 commits January 29, 2024 12:53

Apply suggestions from code review

6316b88

Co-authored-by: awildturtok <1553491+awildturtok@users.noreply.github.com>

Review changes

35be28c

Merge branch 'develop' into feature/ngram-search

2dc5ba7

awildturtok commented Jan 31, 2024

View reviewed changes

SebChmie and others added 12 commits February 1, 2024 10:13

Merge branch 'develop' into feature/ngram-search

1dc59ef

it is okay for shrinkToFit to change search behaviour

ee2700e

Remove comment

6488a19

Fix calculateSize

d3732eb

adds fixes a bug, where Column and SelectFilter where not shrunk afte…

c62e1bc

…r creation

use barrier on insertion of single item

83a4fd6

Log durations

3edbacf

simplify ngram lookup

c60dc88

Fix missing items

e0f7ca7

use PatriciaTrie in ngram lookup for words smaller than ngramLength

1441808

Cleanup timer logging

3d5877b

just use the timer

f541316

SebChmie added 7 commits February 8, 2024 17:37

Set duration-logs level to TRACE

8ba64cf

Fix tests

dadf53c

Simplify weight function

7341f5c

Simplify weight function

7545f15

Merge branch 'develop' into feature/ngram-search

f43cc6a

Collection::iterator

ab8805c

Simplify weight function

e384190

SebChmie requested a review from thoniTUB February 14, 2024 08:49

thoniTUB requested changes Feb 14, 2024

View reviewed changes

awildturtok and others added 2 commits February 14, 2024 17:48

Update backend/src/main/java/com/bakdata/conquery/models/index/IndexS…

eaa9385

…ervice.java Co-authored-by: MT <12283268+thoniTUB@users.noreply.github.com>

Update backend/src/main/java/com/bakdata/conquery/models/index/Fronte…

6a79739

…ndValueIndex.java Co-authored-by: MT <12283268+thoniTUB@users.noreply.github.com>

awildturtok commented Feb 15, 2024

View reviewed changes

SebChmie added 2 commits February 15, 2024 10:58

Review changes

1e0d6a6

Add documentation

372f23f

awildturtok commented Feb 15, 2024

View reviewed changes

SebChmie and others added 3 commits February 16, 2024 10:22

Apply suggestions from code review

fc1e761

Co-authored-by: awildturtok <1553491+awildturtok@users.noreply.github.com>

Fix code review changes

7ea0d62

Merge branch 'develop' into feature/ngram-search

2370daa

SebChmie merged commit 95bb0b7 into develop Feb 16, 2024
6 checks passed

delete-merged-branch bot deleted the feature/ngram-search branch February 16, 2024 10:21

thoniTUB reviewed Feb 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use ngram in TrieSearch as it performs very similarly but at much low… #3216

use ngram in TrieSearch as it performs very similarly but at much low… #3216

awildturtok commented Nov 15, 2023

awildturtok Jan 18, 2024

awildturtok Jan 18, 2024

awildturtok Jan 18, 2024

awildturtok Jan 18, 2024

awildturtok Jan 18, 2024

awildturtok Jan 18, 2024

awildturtok left a comment

awildturtok Jan 24, 2024

awildturtok Jan 24, 2024

awildturtok Jan 24, 2024

awildturtok Jan 24, 2024

awildturtok Jan 24, 2024

SebChmie Jan 24, 2024

awildturtok Jan 29, 2024

awildturtok Jan 24, 2024

awildturtok Jan 24, 2024

awildturtok Jan 29, 2024

awildturtok Jan 29, 2024

SebChmie Jan 29, 2024 •

edited

SebChmie Jan 29, 2024 •

edited

awildturtok Jan 29, 2024

awildturtok Jan 31, 2024

thoniTUB Feb 14, 2024

awildturtok Feb 14, 2024

SebChmie Feb 15, 2024

awildturtok Feb 15, 2024

SebChmie Feb 15, 2024

awildturtok left a comment

awildturtok Feb 15, 2024

awildturtok commented Feb 15, 2024

thoniTUB Feb 22, 2024

awildturtok Feb 22, 2024

		// This barrier avoids work when shrinking and therefore unnecessary calls to entries::computeIfAbsent
		final Set<String> barrier = new HashSet<>();

use ngram in TrieSearch as it performs very similarly but at much low… #3216

use ngram in TrieSearch as it performs very similarly but at much low… #3216

Conversation

awildturtok commented Nov 15, 2023

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

awildturtok 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

SebChmie Jan 29, 2024 • edited

Choose a reason for hiding this comment

SebChmie Jan 29, 2024 • edited

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

awildturtok left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awildturtok commented Feb 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebChmie Jan 29, 2024 •

edited

SebChmie Jan 29, 2024 •

edited