Fix for Maintain Table Formatting in Markdown Files #1645

[pdubey28-sketch]

Pull Request — Fix for Maintain Table Formatting in Markdown Files (#1645)

Branch: pdubey28-sketch:Issue#1645 → pdubey28-sketch:swift
Commit: c8be3eb6066118b76bc2c2ae3d1d0f0308658e8d (single commit, 4 files, +256 / −26)

---

1. What problem does this solve — and where is it tracked?

ID: no ID — mechanical fix. GitHub issue ThalesGroup/fred#1645 is the source of truth; no entry exists in docs/swift/data/id-legend.yaml. If tracking is wanted, propose adding a RUNTIME-xx row (adapter dedup/sort) and a knowledge-flow ingestion row before merge.

Problem in one sentence: Markdown tables ingested into the RAG store were returned to the LLM out of document order and with broken header/row structure, so row-level questions over tables (e.g. "what is the default of property25?") returned wrong or empty answers.

Backlog ref: none — no [ ] item existed for this bug. Recommend adding one before merge.

---

2. Before you wrote a single line of code

RFC written? Not required — mechanical fix. The change restores the documented "preserve tables" behaviour of SemanticSplitter for .md/.txt inputs and repairs a half-applied chunk-ordering fix in the v2 runtime adapter. No new contract or endpoint; one optional internal DTO field (VectorSearchHit.chunk_index) that the adapter was already trying to read.

Plan presented to the team before implementation? No — the bug was diagnosed and patched in a single debugging session from a reproducible failure (large-table-test_pd1.md returning tables in similarity-score order with repeated headers). The plan is captured retroactively in docs/MARKDOWN_TABLE_INGESTION_FIX.md.

Confirmation received? No prior confirmation — this PR is the request for review.

---

3. What you built

One commit (c8be3eb) with two coordinated fixes for the same regression:

1. Ingestion — SemanticSplitter. Added _auto_annotate_unmarked_tables so plain-markdown and text inputs get the same <!-- TABLE_START --> / <!-- TABLE_END --> wrapping that PdfMarkdownProcessor and DocxMarkdownProcessor already emit (detection is conservative: a pipe row must be immediately followed by a markdown separator row; fenced code blocks and quoted blocks are skipped). Tables that fit in chunk_size are emitted as a single atomic chunk. Oversized tables fall back to _split_large_table, which splits only at row boundaries and repeats the header + separator in every part, stamping block_type="markdown_table", table_id, table_part, row_start, row_end.
2. Retrieval — VectorSearchHit + _to_hit + _invoke_knowledge_search. Added the missing chunk_index: Optional[int] = None field to VectorSearchHit, populated it from doc metadata in VectorSearchService._to_hit(), and added a prev_content_is_table guard to the dedup loop in the v2-runtime adapter so the first table chunk keeps its header even when an intro/text chunk from the same document precedes it. The sort by chunk_index in _invoke_knowledge_search was already in place but silently raising AttributeError; it now works.

Files changed

File	Lines
apps/knowledge-flow-backend/knowledge_flow_backend/core/processors/output/vectorization_processor/semantic_splitter.py	+204 / −26
apps/knowledge-flow-backend/knowledge_flow_backend/features/vector_search/vector_search_service.py	+4 / −0
libs/fred-core/fred_core/store/vector_search.py	+1 / −0
libs/fred-runtime/fred_runtime/integrations/v2_runtime/adapters.py	+47 / −0

---

4. Proof of quality

Tests added or updated:

Test	File	What it covers
test_small_table_remains_in_one_chunk	apps/knowledge-flow-backend/tests/processors/output/vectorization_processor/test_semantic_splitter_tables.py	Small table stays whole
test_text_before_and_after_table_chunked_separately	same file	Prose chunks normally; table whole; no marker leakage
test_large_table_splits_only_on_row_boundaries_and_repeats_header	same file	Row-boundary splits only; header+separator repeated in every part; rows in original order
test_split_preserves_row_order_in_metadata	same file	row_start/row_end contiguous and monotonic
test_multiple_tables_are_each_preserved_independently	same file	Distinct ids per table, both preserved
test_pipe_text_without_separator_row_is_not_treated_as_table	same file	Pipe prose without separator is left as prose
test_fenced_code_block_with_pipes_is_not_treated_as_table	same file	Code-fence pipes not misdetected
test_auto_annotation_does_not_misdetect_separator_inside_text	same file	Separator-like prose without pipes not misdetected
test_pre_annotated_tables_are_idempotent	same file	Existing PDF/DOCX annotations keep their ids and content

Retrieval-side change is covered by 5 manual assertions in C:\tmp\probe_fix_verify.py (all pass). No formal pytest exists yet for FredKnowledgeSearchToolInvoker._invoke_knowledge_search (would require mocking the vector-search HTTP client) — recommended as a follow-up.

make code-quality output:

TODO — paste output from:
  (cd apps/knowledge-flow-backend && make code-quality)
  (cd libs/fred-core && make code-quality)
  (cd libs/fred-runtime && make code-quality)

Raw basedpyright output (required if a touched package keeps a non-empty baseline file):

TODO — run basedpyright for each touched package and paste output
if a baseline file is present.

make test output:

test_semantic_splitter_tables.py — 9/9 PASS

TODO — paste `make test` summary line for:
  apps/knowledge-flow-backend
  libs/fred-core
  libs/fred-runtime

---

5. Docs updated

What changed	File updated
Backlog[ ] item is now done	n/a — no backlog item existed for issue #1645
New behaviour, API field, or contract change	docs/MARKDOWN_TABLE_INGESTION_FIX.md (new) — splitter behaviour + auto-annotation rules
Frozen contract touched (execution.py, agent_app.py, OpenAPI)	n/a —VectorSearchHit.chunk_index is an optional internal DTO field
UX component implemented or visual status changed	n/a
Phase progress row exists for this area	n/a
WORKPLAN sprint item finished	n/a
Code and a design doc now diverge	n/a

---

6. Close-out statement

## Task close-out
- Code:
    - SemanticSplitter: auto-annotate unmarked markdown tables; atomic
      table chunk when it fits; row-boundary split with repeated
      header+separator when oversized
    - VectorSearchHit: added optional `chunk_index` field
    - VectorSearchService._to_hit: pass `chunk_index` from metadata
    - v2_runtime adapter `_invoke_knowledge_search`: dedup guard so the
      first table chunk after an intro chunk keeps its header
- Tests: 9 new ingestion tests in
    apps/knowledge-flow-backend/tests/processors/output/vectorization_processor/test_semantic_splitter_tables.py
    (all pass); 5 probe assertions for the retrieval-side change (pass);
    no formal adapter pytest added (follow-up).
- Docs updated: docs/MARKDOWN_TABLE_INGESTION_FIX.md (new)
- Backlog: none — no [ ] item existed; recommend adding one before merge
- Skipped steps: Step 1 (RFC) — mechanical fix; Step 2 (backlog entry)
    — no pre-existing item; Step 3 (developer confirmation) — diagnosed
    and patched from a reproducible failure in one session, this PR is
    the request for confirmation; Step 3.5 (GitHub issue) — #1645 exists.

---

7. Risk and rollback

What breaks if this is wrong?

• Splitter false positives. If _auto_annotate_unmarked_tables misclassifies pipe-using prose as a table, that block becomes a single atomic chunk instead of being prose-split. Detection is conservative (pipe row must be immediately followed by a markdown separator row of only |, -, :, whitespace) and covered by test_pipe_text_without_separator_row_is_not_treated_as_table and test_fenced_code_block_with_pipes_is_not_treated_as_table.
• Old documents without chunk_index. The adapter sort falls back to 999_999 for hits missing chunk_index, so legacy-indexed documents sort to the end of the result list. Acceptable degradation; re-ingestion restores correct ordering.
• Dedup over-stripping. The prev_content_is_table guard strips a header only when the immediately preceding same-document chunk was also a table chunk. Worst case: a continuation chunk keeps a duplicate header (cosmetic), not a wrong answer.

How do we roll back?

Revert commit c8be3eb (the four touched files + the new test file). The splitter change is process-local — restart knowledge-flow-backend after revert. Already-ingested documents do not need re-ingestion to roll back (the new metadata fields are additive), but tables ingested under the new splitter will revert to the previous broken behaviour on next retrieval.

[dimitri-tombroff]

Hi @pdubey28-sketch 👋 — thanks a lot for this, and especially for the really thorough write-up. The document-order fix and the row-boundary table splitting are exactly the right approach, and the conservative detection rules are nicely done. 🙌

Just two things I'd love your help understanding before we merge — both questions, not blockers:

1. The preserve_tables flag. In the previous reinsert logic, preserve_tables=True kept a large table as a single atomic chunk, and only False allowed it to be split. The new code always splits when the table is larger than chunk_size, so the flag isn't consulted in that path anymore. Was that an intentional simplification (i.e. we always want row-boundary splitting now)? If so, all good — could you just confirm, so we can note it? If not, we may want to keep honoring the flag.

2. Table detection vs. header dedup. Your _is_pipe_row detection nicely handles tables without a leading pipe, but the dedup in the runtime adapter decides "is this a table chunk?" with hit.content.startswith("|"). For a leading-pipe-less table, that check would miss it and the repeated header wouldn't get stripped. Is that case in scope, or are we assuming all ingested tables are normalized to leading-pipe form? Related: could you confirm chunk_index is actually populated in the chunk metadata for these table sub-chunks? The splitter stamps table_part / table_chunk_id, and I want to make sure the new chunk_index sort in the adapter isn't quietly falling back to the 999_999 default for them.

No rush on any of this — happy to pair on it if that's easier. Thanks again for the great contribution! 🚀

[pdubey28-sketch]

@dimitri-tombroff Please find my answers inline

1. preserve_tables Flag (Dead Code)
   preserve_tables is no longer used by the semantic chunking logic, making it dead code after the Maintain Table Formatting in Markdown Files #1645 refactor.
   Regardless of the flag value:
   Small tables (≤ chunk_size) are always kept as a single chunk.
   Large tables are always split at row boundaries using _split_large_table().
   Since all production and test callers already use the default (True), there is no functional regression.

Q1 – Leading Pipe (|) Table Detection Mismatch
A pre-existing inconsistency exists for raw Markdown tables without a leading |.
Table detection accepts any row containing |, but deduplication only recognizes chunks starting with |, so repeated headers may not be removed.
This is not related to Issue #1774 and does not affect current tests (large-table-test_pd1.md) or normal ingestion, as DOCX/PDF/CSV processors already normalize tables to the standard leading-pipe format.
Recommendation: Track this as a separate issue and normalize leading pipes during _auto_annotate_unmarked_tables instead of changing the runtime dedup logic.

Q2 – chunk_index Population
chunk_index is correctly populated during vectorization, not during table splitting.
It survives metadata sanitization and is stored in OpenSearch, making it available for retrieval sorting.
Temporary metadata (table_part, table_chunk_id) is intentionally discarded before storage.
The 999999 fallback is only used for:
Old indexes created before chunk_index was stored (requires re-indexing), or
Unit tests that bypass the vectorization pipeline.
For all freshly indexed documents, including table sub-chunks, chunk_index is present and the fallback is never used.