Fix chunking file_format placement: Markdown-aware chunking was never enabled - #557
Open
claudespice wants to merge 1 commit into
Open
Conversation
file_format belongs in the dataset's params, not inside the chunking block. Inside chunking: it is silently dropped, so Markdown-aware chunking was never enabled.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Three recipes set
file_format: mdinside thechunking:block of a column embedding:chunking:deserializes intoEmbeddingChunkConfig, which has exactly four fields —enabled,target_chunk_size,overlap_size,trim_whitespace— and nofile_format. The struct does not carry#[serde(deny_unknown_fields)], so the key is silently dropped rather than reported: the recipes look like they enable Markdown-aware chunking, but every one of them has been chunking with the plain text splitter.The runtime reads
file_formatfrom the dataset'sparams, not from the chunking block. This PR moves the key up intoparams:, which is also the placement used by thefilerecipe (file/README.md) and by spiceai's own test spicepods (test/models/spicepod_openai.yml).Recipes fixed:
models/openai(spicepod.yaml+ the README's copy of the same block)azure_openai(spicepod.yaml)search_github_files(spicepod.yaml, commented-out example)Verified against
spiceai/spiceai at
v2.1.1:crates/spicepod/src/component/embeddings.rs—EmbeddingChunkConfighas onlyenabled/target_chunk_size/overlap_size/trim_whitespace; nodeny_unknown_fields.crates/runtime-search/src/embeddings/table.rs—from_spicepod_columnsbuildsChunkingConfigfrom the per-column chunk config for the three size fields, and takesfile_formatfrom a separate argument.crates/runtime/src/embeddings/connector.rs— that argument isdataset.params.get("file_format").crates/runtime/src/component/dataset/builder.rs—Dataset.paramsis the raw spicepod params map, so the chunker sees the key regardless of connector parameter specs.crates/chunking/src/lib.rs—match cfg.file_format { Some("md" | ".md" | "mdx" | ".mdx") => Splitter::Markdown(...), _ => Splitter::Text(...) }.git log -S file_formatconfirms the misplacement predates #508 and #136, which only flippedchunking.enabledfromfalsetotrue— so this is not reverting a deliberate choice.One thing for reviewers to weigh
The
githubconnector does not listfile_formatin itsPARAMETERSspec, so it will log one line at startup:This is cosmetic — the chunker reads the raw params map, so Markdown chunking does take effect — but if you would rather not have the warning, the alternative is to drop the
file_formatline entirely and accept plain-text chunking (which is what these recipes have actually been doing all along). Happy to switch to that if preferred.Evidence
Static review only — these recipes need a
GITHUB_TOKENplus OpenAI/Azure credentials, so they were not run. The config path is traced end-to-end in the source links above, and all three files were re-parsed as valid YAML after the change.