diff --git a/internal/ingestion/component/tokenizer.go b/internal/ingestion/component/tokenizer.go index 13b70e3159..dcdcc5bcf2 100644 --- a/internal/ingestion/component/tokenizer.go +++ b/internal/ingestion/component/tokenizer.go @@ -413,6 +413,13 @@ func (c *TokenizerComponent) embedChunks(ctx context.Context, tenantID, kbID, em tokenCount int hasTitleVec bool ) + // go_intentional (A3): when the upstream name is empty we skip title + // weighting entirely (hasTitleVec stays false, so the merged vector is the + // content vector alone). From the end-user perspective an empty title must + // not contribute the filename embedding weight; the Python DSL instead + // computes 0.1*emb(""), injecting an undefined bias into every chunk. Go's + // skip is the correct behavior (go_intentional). Do NOT "align" this to + // the DSL. if trimmedName == "" { log.Printf("Tokenizer: empty name provided from upstream, embedding will skip title weighting") } else { @@ -680,13 +687,11 @@ func tokenizeChunks(chunks []schema.ChunkDoc, titleStem string, language string) } } if kw := ck.Keywords; kw != "" { - // A2: split on the ENGLISH COMMA only, matching the DSL tokenizer - // (rag/flow/tokenizer/tokenizer.py:153 `keywords.split(",")`) and - // the keyword_prompt contract ("delimited by ENGLISH COMMA"). CJK - // commas/semicolons and newlines stay part of the keyword so the - // Go index is byte-compatible with the Python-DSL-built index. - // strings.Split preserves empty elements, matching Python's - // "a,,b".split(",") == ["a","","b"]. + // Split keywords on the ENGLISH COMMA ONLY. The keyword_prompt + // contract specifies "delimited by ENGLISH COMMA", so CJK commas, + // semicolons and newlines stay part of the keyword rather than + // acting as separators. strings.Split also preserves empty + // elements, matching Python's "a,,b".split(",") == ["a","","b"]. if err = ck.SetExtraValue("important_kwd", strings.Split(kw, ",")); err != nil { return fmt.Errorf("tokenizer: keyword list marshal: %w", err) } @@ -772,6 +777,13 @@ func concatFields(ck schema.ChunkDoc, fields []string) string { // shouldHaveEmbedding reports whether the tokenizer must attach embedding // vectors: the search method requests embedding AND a KB is present. +// +// go_intentional (A4): the kbID != "" guard is deliberate. Each dataset +// configures its own embedding model, so an empty kb_id (e.g. a canvas-debug +// dry run) must NOT fall back to the tenant's default embedding model — doing +// so would produce vectors a dataset cannot actually use at retrieval time. +// This is a deliberate, go_intentional divergence. Do NOT "align" this to a +// path that injects a default embedding. func shouldHaveEmbedding(searchMethods []string, kbID string) bool { return contains(searchMethods, "embedding") && kbID != "" } diff --git a/internal/ingestion/component/tokenizer_unit_test.go b/internal/ingestion/component/tokenizer_unit_test.go index 2454ea79f8..985dd48981 100644 --- a/internal/ingestion/component/tokenizer_unit_test.go +++ b/internal/ingestion/component/tokenizer_unit_test.go @@ -702,3 +702,115 @@ func TestTokenizerComponent_ImportantKwd_CommaOnly(t *testing.T) { t.Errorf("important_tks = %v, want full keyword string", got[0]["important_tks"]) } } + +// TestTokenizerComponent_ImportantKwd_PreservesEmptyElements locks F3: the +// component path splits on the ENGLISH COMMA ONLY via strings.Split and +// PRESERVES empty elements, matching Python's "a,,b".split(",") == +// ["a","","b"]. This is the contract asserted by the comment at +// tokenizer.go:688-689, and it is the intentional counterpart to the executor +// fallback (cleanupConsumedChunkFields -> utility.SplitKeywords) which DROPS +// empty parts. Runs without the C++ analyzer pool via the identity engine. +func TestTokenizerComponent_ImportantKwd_PreservesEmptyElements(t *testing.T) { + tokenizer.SetEngineType("infinity") + defer tokenizer.SetEngineType("") + + c, err := NewTokenizerComponent(map[string]any{ + "search_method": []any{"full_text"}, + }) + if err != nil { + t.Fatalf("NewTokenizerComponent: %v", err) + } + + // Middle empty element must be preserved (["a","","b"]), not dropped. + out, err := c.Invoke(context.Background(), nil, map[string]any{ + "output_format": "chunks", + "chunks": []map[string]any{ + {"text": "doc body", "keywords": "a,,b"}, + }, + }) + if err != nil { + t.Fatalf("Invoke: %v", err) + } + got, ok := out["chunks"].([]map[string]any) + if !ok || len(got) != 1 { + t.Fatalf("chunks = %v, want 1 chunk", out["chunks"]) + } + kwd, ok := got[0]["important_kwd"].([]string) + if !ok { + t.Fatalf("important_kwd should be []string, got %T", got[0]["important_kwd"]) + } + want := []string{"a", "", "b"} + if len(kwd) != len(want) { + t.Fatalf("important_kwd = %v, want %v (empty elements preserved)", kwd, want) + } + for i := range want { + if kwd[i] != want[i] { + t.Errorf("important_kwd[%d] = %q, want %q (empty elements preserved)", i, kwd[i], want[i]) + } + } + + // Empty keyword string must not error. The component guards on a + // non-empty keywords field (tokenizer.go:682), so important_kwd is left + // unset (nil) rather than materialized as an empty array. Downstream + // indexing treats a missing important_kwd as "no keywords", which is safe. + outEmpty, err := c.Invoke(context.Background(), nil, map[string]any{ + "output_format": "chunks", + "chunks": []map[string]any{ + {"text": "doc body", "keywords": ""}, + }, + }) + if err != nil { + t.Fatalf("Invoke(empty keywords): %v", err) + } + gotEmpty, ok := outEmpty["chunks"].([]map[string]any) + if !ok || len(gotEmpty) != 1 { + t.Fatalf("empty chunks = %v, want 1 chunk", outEmpty["chunks"]) + } + if kwd, exists := gotEmpty[0]["important_kwd"]; exists && kwd != nil { + t.Errorf("empty keywords should leave important_kwd unset, got %v", kwd) + } +} + +// TestTextPayloadToChunks_B1B2 pins the text-payload adapter used for +// markdown/text/html payloads (tokenizer.go:578). This is the Go-correct +// counterpart to a Python-DSL bug where turning full_text off dropped the +// entire document (B1), and where an empty payload failed to emit the chunks +// key at all (B2). The Go helper always returns a slice: non-empty payload -> +// one chunk carrying the raw text; nil/empty/whitespace payload -> a non-nil +// EMPTY slice so the downstream "chunks" key is present as []. +// +// No-tag unit test: textPayloadToChunks is a pure function, no CGo pool needed. +func TestTextPayloadToChunks_B1B2(t *testing.T) { + // B1: a real payload yields exactly one chunk with the text preserved. + payload := "plain payload body" + got := textPayloadToChunks(&payload) + if len(got) != 1 { + t.Fatalf("non-empty payload: len(chunks) = %d, want 1", len(got)) + } + if got[0].Text != payload { + t.Errorf("chunk text = %q, want %q", got[0].Text, payload) + } + + // B2: empty/whitespace/nil payloads must still return a non-nil EMPTY + // slice (so the chunks key is present as [] downstream), never nil. + for _, tc := range []struct { + name string + in *string + }{ + {"empty string", ptr("")}, + {"whitespace only", ptr(" \n\t ")}, + {"nil pointer", nil}, + } { + t.Run(tc.name, func(t *testing.T) { + out := textPayloadToChunks(tc.in) + if out == nil { + t.Fatalf("textPayloadToChunks(%s) = nil, want non-nil empty slice", tc.name) + } + if len(out) != 0 { + t.Errorf("textPayloadToChunks(%s) len = %d, want 0", tc.name, len(out)) + } + }) + } +} + +func ptr(s string) *string { return &s } diff --git a/internal/ingestion/task/indexdoc/process_test.go b/internal/ingestion/task/indexdoc/process_test.go index 168f040098..591f659444 100644 --- a/internal/ingestion/task/indexdoc/process_test.go +++ b/internal/ingestion/task/indexdoc/process_test.go @@ -345,3 +345,67 @@ func TestProcessChunkPositions_NoPositions(t *testing.T) { t.Error("_pdf_positions must be pruned even when positions is missing") } } + +// TestCleanupConsumedChunkFields_ImportantKwdMultiDelimiter pins the executor +// fallback's important_kwd materialization. When the Tokenizer component did +// NOT pre-produce important_kwd, the executor falls back to +// utility.SplitKeywords, which splits on the full delimiter set +// (ASCII + CJK comma/semicolon/ideographic-comma/newline) and DROPS empty +// parts. This is intentionally different from the Tokenizer component path +// (internal/ingestion/component/tokenizer.go:690), which splits on the ENGLISH +// COMMA ONLY and PRESERVES empty elements to match the DSL +// (rag/flow/tokenizer/tokenizer.py:153 `keywords.split(",")`). +// +// The two layers deliberately diverge: the component aligns to the DSL keyword +// contract ("delimited by ENGLISH COMMA"); the executor fallback mirrors +// Python task_executor.run_dataflow:879 and tolerates mixed delimiters from +// older upstream producers. Neither side should be "unified" to the other — +// changing one without the other silently breaks the documented parity +// boundary. The component-side half of this contract is locked by +// TestTokenizerComponent_ImportantKwd_CommaOnly in the component package. +func TestCleanupConsumedChunkFields_ImportantKwdMultiDelimiter(t *testing.T) { + ck := map[string]any{"text": "hello", "keywords": "kw1,kw2;kw3,kw4"} + + cleanupConsumedChunkFields(ck) + + kwd, ok := ck["important_kwd"].([]string) + if !ok { + t.Fatalf("important_kwd should be []string, got %T", ck["important_kwd"]) + } + // Executor fallback splits on comma/semicolon/CJK-comma and drops empties: + // "kw1,kw2;kw3,kw4" -> ["kw1","kw2","kw3","kw4"], NOT the component's + // ["kw1","kw2;kw3,kw4"]. + want := []string{"kw1", "kw2", "kw3", "kw4"} + if len(kwd) != len(want) { + t.Fatalf("executor important_kwd = %v, want %v (multi-delimiter, empties dropped)", kwd, want) + } + for i := range want { + if kwd[i] != want[i] { + t.Errorf("executor important_kwd[%d] = %q, want %q", i, kwd[i], want[i]) + } + } + if _, exists := ck["keywords"]; exists { + t.Error("keywords source field should be consumed/removed") + } +} + +// TestCleanupConsumedChunkFields_ImportantKwdDropsEmptyParts documents that the +// executor fallback drops empty parts (e.g. the middle empty token in +// "a,,b"), diverging from the component path which PRESERVES it as ["a","","b"]. +// Together with the component CommaOnly test this locks the intentional +// divergence: same input, different important_kwd arrays per layer. +func TestCleanupConsumedChunkFields_ImportantKwdDropsEmptyParts(t *testing.T) { + ck := map[string]any{"text": "hello", "keywords": "a,,b"} + + cleanupConsumedChunkFields(ck) + + kwd, ok := ck["important_kwd"].([]string) + if !ok { + t.Fatalf("important_kwd should be []string, got %T", ck["important_kwd"]) + } + // Executor drops the empty middle part: ["a","b"], NOT ["a","","b"]. + want := []string{"a", "b"} + if len(kwd) != len(want) || kwd[0] != "a" || kwd[1] != "b" { + t.Fatalf("executor important_kwd = %v, want %v (empty parts dropped)", kwd, want) + } +} diff --git a/internal/utility/split.go b/internal/utility/split.go index 0334fa09d3..b9abc947af 100644 --- a/internal/utility/split.go +++ b/internal/utility/split.go @@ -46,10 +46,17 @@ func nonEmpty(parts []string) []string { } // SplitKeywords splits a keywords string by common (ASCII + CJK) delimiters, -// dropping empty elements. Returns nil for an empty input. It is the single -// authority for materializing the important_kwd array from the keywords -// string, shared by the Tokenizer component (in-pipeline) and the executor's -// persist-schema mapping. Mirrors Python task_executor.run_dataflow:879. +// dropping empty elements. Returns nil for an empty input. It is the authority +// for materializing the important_kwd array in the executor's persist-schema +// mapping (rag/flow/task_executor.py run_dataflow:879), which tolerates mixed +// delimiters from older upstream producers. +// +// NOTE: the in-pipeline Tokenizer component does NOT use this function. For +// DSL byte-compatibility it splits keywords on the ENGLISH COMMA ONLY via +// strings.Split(kw, ",") and PRESERVES empty elements (mirroring Python +// rag/flow/tokenizer/tokenizer.py:153 "a,,b".split(",")), so the two paths +// intentionally diverge. See tokenizeChunks in +// internal/ingestion/component/tokenizer.go. func SplitKeywords(keywords string) []string { if keywords == "" { return nil