Text splitter implementations #77

AyoTheDev · 2023-05-24T15:24:17Z

No description provided.

li2109

Let's add some comments. at least a javadoc or simple comment for each public class.

sometimes, I was also skipping some comments, but let's aim for it.

li2109 · 2023-05-24T18:27:33Z

src/main/java/ai/knowly/langtorch/parser/textsplitter/CharacterTextSplitter.java

@@ -0,0 +1,30 @@
+package ai.knowly.langtorch.parser.textsplitter;


ai.knowly.langtorch.preprocessing.splitter.text?

Could you also move the parser into this preprocessing? I feel like we probably have image, csv, etc stuff. Like pdf for pdf chatbot very soon.

li2109 · 2023-05-24T18:29:55Z

src/test/java/ai/knowly/langtorch/parser/TextSplitterTest.java

+
+    @Test
+    public void testCharacterTextSplitter_splitByCharactersSplitsNotFoundEasily() {
+        // Arrange


nit: comma at the end.
i.e. // Arrange => // Arrange.

dito for other comments.

li2109

As i found out there's already implemented util methods from Apache commons, i didnt the RecursiveCahracterTextSplitter and the test. I think we probably can import many methods from that library which significantly reduce the code size

li2109 · 2023-05-24T19:17:38Z

src/main/java/ai/knowly/langtorch/parser/textsplitter/TextSplitter.java

+    abstract public List<String> splitText(String text);
+
+    public List<DomainDocument> createDocuments(List<String> texts, @Nullable List<Map<String, String>> metaDatas) {
+        List<Map<String, String>> _metadatas;


java variable name should be in lowerCamelCase

https://google.github.io/styleguide/javaguide.html#s5.2.7-local-variable-names

li2109 · 2023-05-24T19:19:12Z

src/main/java/ai/knowly/langtorch/parser/textsplitter/TextSplitter.java

+
+    abstract public List<String> splitText(String text);
+
+    public List<DomainDocument> createDocuments(List<String> texts, @Nullable List<Map<String, String>> metaDatas) {


create a java object for metaDatas and make it optional(then we can remove @nullable)

li2109 · 2023-05-24T19:21:23Z

src/main/java/ai/knowly/langtorch/parser/textsplitter/TextSplitter.java

+                int newLinesCount = chunk.split("\n").length - 1;
+
+                Map<String, String> loc;
+                //todo should we also check what type of object is "loc"?


todo should looks like this:
// TODO: Handle null user condition.

li2109 · 2023-05-24T19:25:51Z

src/main/java/ai/knowly/langtorch/parser/textsplitter/TextSplitter.java

+        }
+        ArrayList<DomainDocument> documents = new ArrayList<>();
+
+        for (int i = 0; i < texts.size(); i += 1) {


for(String text: texts)

We need the index later on though

li2109 · 2023-05-24T19:49:56Z

src/main/java/ai/knowly/langtorch/parser/textsplitter/CharacterTextSplitter.java

What's the difference between CharacterTextSplitter and RecursiveCharacterTextSplitter? bit lost and should have more comments

I've added comments explaining

li2109 · 2023-05-24T19:58:59Z

src/main/java/ai/knowly/langtorch/parser/textsplitter/TextSplitter.java

checkout org.apache.commons:commons-lang3 which contains many util methods we can use here.

https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lang3/StringUtils.html

There are some limits on where we can use this in the recursive character splitter. the recursion causes a stack overflow error

li2109 · 2023-05-25T16:10:19Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs 0 Vulnerabilities 0 Security Hotspots 12 Code Smells

0.0% Coverage 0.0% Duplication

Please also resolve the code smell check here.

li2109

review is not yet done but I will continue later today

li2109 · 2023-05-25T15:58:12Z

build.gradle

@@ -116,6 +116,9 @@ dependencies {
    testImplementation 'com.squareup.retrofit2:retrofit-mock:2.9.0'
    implementation 'com.squareup.okhttp3:logging-interceptor:4.9.2'
    implementation "com.squareup.retrofit2:converter-gson:2.9.0"
+    // Apache commons lang
+    implementation group: 'org.apache.commons', name: 'commons-lang3', version: '3.0'
+


implementation org.apache.commons:commons-lang3:3.0 ? to make it consistent with other dependency?

li2109 · 2023-05-25T15:58:50Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/CharacterTextSplitter.java

+
+public class CharacterTextSplitter extends TextSplitter {
+
+    public String separator = "\n\n";


private static final String SEPARATOR = ...

We want to give the developer the option to overwrite the default value via the constructor, I've made it static though

sorry i didnt notice you are trying to change it later. please remove static then if that's the case.

li2109 · 2023-05-25T16:01:56Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/CharacterTextSplitter.java

+            splits = Arrays.asList(StringUtils.splitByWholeSeparatorPreserveAllTokens(text, this.separator));
+        } else {
+            splits = Arrays.asList(StringUtils.splitByWholeSeparatorPreserveAllTokens(text, ""));
+        }


splits = Arrays.asList(StringUtils.splitByWholeSeparatorPreserveAllTokens(text, this.separator.isEmpty() ? "" : this.separator));

li2109 · 2023-05-25T16:04:02Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/CharacterTextSplitter.java

+    public List<String> splitText(String text) {
+        List<String> splits;
+
+        if (StringUtils.isNotEmpty(this.separator)) {


be careful of this method which is not the same as isEmpty in String class

li2109 · 2023-05-25T16:06:09Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitter.java

+        int total = 0;
+
+        for (String d : splits) {
+            int _len = d.length();


len = d.length();

we dont use _ to indicate it's a private variable like in python,

li2109 · 2023-05-25T16:06:58Z

src/main/java/ai/knowly/langtorch/schema/io/Metadatas.java

+
+public class Metadatas {
+
+    private final List<Map<String, String>> values;


use Multimap instead if there's more than one value associated with the same key

oh nice, was not aware of this. It's why before I was thinking we might need to nest a map

li2109 · 2023-05-25T16:07:46Z

src/test/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitterTest.java

+        int chunkSize = 2;
+        int chunkOverlap = 4;
+
+        // Act


li2109 · 2023-05-25T16:08:33Z

src/test/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitterTest.java

+                }})
+        );
+
+        assertEquals(expectedDocs.size(), docs.size());


let's use assertThat from google truth library and you can find example in other tests to be consistent

li2109 · 2023-05-25T16:11:34Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitter.java

+
+    public int chunkSize;
+
+    public int chunkOverlap;


make these two fina variable if you are not trying to modify it later

li2109 · 2023-05-27T02:48:59Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitter.java

+
+            if (total + length + (currentDoc.size() > 0 ? separator.length() : 0) > this.chunkSize) {
+                if (total > this.chunkSize) {
+                    System.out.println("Created a chunk of size " + total + ", which is longer than the specified " + this.chunkSize);


use logger instead.

example: src/main/java/ai/knowly/langtorch/example/SimpleChatBotWithoutExplicitKey.java

li2109 · 2023-05-27T02:49:48Z

src/test/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitterTest.java

+        // Assert.
+        List<String> expected = new ArrayList<>(Arrays.asList("a a", "foo", "bar", "baz"));
+
+        Truth.assertThat(Objects.equals(expected, result));


just use assertThat(It's a static function and you can import it directly) and remove Truth.

li2109 · 2023-05-27T02:50:45Z

src/test/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitterTest.java

+        // Assert.
+        List<String> expected = new ArrayList<>(Arrays.asList("a a", "foo", "bar", "baz"));
+
+        Truth.assertThat(Objects.equals(expected, result));


The assertThat func is not correctly invoked.
assertThat(actual value).isEqualTo(expected value)

li2109 · 2023-05-27T02:51:59Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/CharacterTextSplitter.java

+
+public class CharacterTextSplitter extends TextSplitter {
+
+    public String separator = "\n\n";


sorry i didnt notice you are trying to change it later. please remove static then if that's the case.

li2109 · 2023-05-27T02:56:36Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitter.java

+
+    public final int chunkOverlap;
+
+    public TextSplitter(int chunkSize, int chunkOverlap) {


make it protected. Constructors of abstract classes can only be called in constructors of their subclasses

li2109 · 2023-05-27T03:14:25Z

src/main/java/ai/knowly/langtorch/schema/io/Metadata.java

+        return value;
+    }
+
+    public static Metadata createEmpty(){


create() is enough

li2109 · 2023-05-27T03:19:14Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitter.java

+
+        ArrayList<DomainDocument> documents = new ArrayList<>();
+
+        for (int i = 0; i < texts.size(); i += 1) {


you are relying on the order to retrieve texts and corresponding metadata based on the assumption that text and docMetadata: 1. it's immutable and 2. order matters in the list.

This is error prone as there's an underlying coupling between these two. why not just create one class that contains both text and metadata so that the input of createDocument will then be a just single list that has no restriction

This makes a lot of sense and simplifies the code

li2109 · 2023-05-27T03:26:14Z

src/main/java/ai/knowly/langtorch/schema/io/Metadata.java

+public class Metadata {
+    private final MultiKeyMap<String, String> value;
+
+    public Metadata(MultiKeyMap<String, String> values) {


if you are doing a fluent style(meaning our API design relies significantly on method chaining) as you did for createEmpty() that returns the class itself, then make it private.

Having a public constructor breaks the "fluent" style.
Why not do Metadata.create().set(a, b).set(c,d);
or if you already having a map, then do Metadata.copyOf(some map)

cool, thanks for the feedback. My only concern with the MultiKey is that it requires a minimum of two keys which we don't always need. For now I'm passing an empty string as one of the keys. What do you think?

li2109 · 2023-05-27T03:44:51Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitter.java

+
+    abstract public List<String> splitText(String text);
+
+    public List<DomainDocument> createDocuments(List<String> texts, Optional<List<Metadata>> docMetadatas) {


could we also create a method called createDocument(...) for single text and metadata

I've renamed this method createDocumentsSplitFromList, have created another method called createDocumentsSplitFromSingle and a private method addDocumentFromWordChunk

li2109 · 2023-05-27T03:54:26Z

src/main/java/ai/knowly/langtorch/preprocessing/splitter/text/TextSplitter.java

+    }
+
+    @Nullable
+    private String joinDocs(List<String> docs, String separator) {


Make it Optional and remove Nullable.

li2109 · 2023-05-27T05:01:28Z

Hi Ayo, I think we should discuss the implementation first before implementing it.

IIUC, CharacterTextSplitter does the split work on a character level.

here's one of your tests:

  @Test
  public void testCharacterTextSplitter_splitByCharacterCountWithNoEmptyDocuments() {
    // Arrange.
    String text = "foo bar";
    CharacterTextSplitter splitter = new CharacterTextSplitter(" ", 2, 0);

    // Act.
    List<String> result = splitter.splitText(text);

    // Assert.
    List<String> expected = new ArrayList<>(Arrays.asList("foo", "bar"));

    Truth.assertThat(result).containsExactlyElementsIn(expected);
  }

I don't get why the expected result is foo and bar. foo and bar are of size 3 which exceed the chunk size 2.

My thought process:
initial: "foo bar"
after separator processing: it becomes foo and bar
with chunk size = 2, and no overlap, it should be fo, ob, and ar
why the result is foo and bar?

Same for

  @Test
  public void testCharacterTextSplitter_splitByCharacterCountLongWords() {
    // Arrange.
    String text = "foo bar baz a a";

    CharacterTextSplitter splitter = new CharacterTextSplitter(" ", 3, 1);

    // Act.
    List<String> result = splitter.splitText(text);

    // Assert.
    List<String> expected = new ArrayList<>(Arrays.asList("foo", "bar", "baz", "a a"));

    Truth.assertThat(Objects.equals(expected, result));
  }

initial string: foo bar baz a a
after separator processing: foo, bar, baz, a, a
with chunk size = 3 and overlap = 1,
it should be foo, oba, arb, baz, zaa

sonarcloud · 2023-05-30T00:26:10Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

0.0% Coverage
0.0% Duplication

li2109 · 2023-05-30T00:33:43Z

I am adding my view of the text splitter implementation. Let's talk before resolving this.

AyoTheDev added 2 commits May 24, 2023 16:20

Text splitter implementations

cbd922a

Added DomainDocument object

d832f5f

li2109 reviewed May 24, 2023

View reviewed changes

li2109 requested changes May 24, 2023

View reviewed changes

AyoTheDev added 2 commits May 25, 2023 07:40

Merge branch 'master' into text-splitter

feb6959

Resolved code review comments

e5c2882

li2109 requested changes May 25, 2023

View reviewed changes

AyoTheDev added 3 commits May 25, 2023 19:34

WIP: responding to code review comments

83b6770

Responded to code review comments

6a0e579

Fixed sonar

a61c474

li2109 requested changes May 27, 2023

View reviewed changes

AyoTheDev and others added 7 commits May 27, 2023 07:31

WIP

bfcdd04

Updated Unit tests

3e6632e

Merge branch 'master' into text-splitter

53f573f

Responded to code review comments

db24a95

Fixed sonar error

70c9dba

fixing code smells

3cb82dc

Adding extra candidate for TextSplitter

a77bb2c

AyoTheDev and others added 2 commits May 30, 2023 07:15

removed old implementation

866daf0

Adding text splitter

c4a52d3

li2109 merged commit 99505de into li2109:master May 30, 2023

AyoTheDev deleted the text-splitter branch June 3, 2023 18:04

		@@ -0,0 +1,30 @@
		package ai.knowly.langtorch.parser.textsplitter;


		abstract public List<String> splitText(String text);

		public List<DomainDocument> createDocuments(List<String> texts, @Nullable List<Map<String, String>> metaDatas) {


		public class CharacterTextSplitter extends TextSplitter {

		public String separator = "\n\n";


		public class Metadatas {

		private final List<Map<String, String>> values;


		public final int chunkOverlap;

		public TextSplitter(int chunkSize, int chunkOverlap) {


		ArrayList<DomainDocument> documents = new ArrayList<>();

		for (int i = 0; i < texts.size(); i += 1) {

Text splitter implementations #77

Text splitter implementations #77

Conversation

AyoTheDev commented May 24, 2023

li2109 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

li2109 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

li2109 commented May 25, 2023

li2109 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

AyoTheDev May 28, 2023 • edited Loading

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

li2109 commented May 27, 2023 • edited Loading

sonarcloud bot commented May 30, 2023

li2109 commented May 30, 2023

AyoTheDev May 28, 2023 •

edited

Loading

li2109 commented May 27, 2023 •

edited

Loading