Enhancing VectorStoreWriter for better RAG support

[Copilot]

This PR addresses two issues in the data ingestion layer:

#6967 - Allow VectorStoreWriter to accept a pre-created VectorStoreCollection: VectorStoreWriter now takes a VectorStoreCollection<Guid, TRecord> directly, allowing users to configure the collection in specific ways. The TKey generic parameter was removed since all MEVD vector collections support Guid keys. Users can also provide their own VectorStoreCollectionDefinition with custom storage names to map to pre-existing collections with different schemas.

#6968 - Figure out the metadata story: SetMetadata now lives on VectorStoreWriter (which is now non-sealed) and throws NotSupportedException by default so that metadata schema mismatches are caught early rather than silently ignored. Users who need metadata must explicitly override SetMetadata in a derived VectorStoreWriter.

Changes

• IngestedChunkRecord.cs: Use nameof() for property names in CreateDefaultCollectionDefinition, assign storage names via StorageName initializer. Remove TKey generic parameter (Key is always Guid). Make storage name constants private (using *StorageName suffix for consistency), except EmbeddingStorageName which is protected so derived types can reference it in [VectorStoreVector] attributes. Remove all JSON attributes and make properties virtual so derived types can annotate as needed. Rename CreateCollectionDefinition to CreateDefaultCollectionDefinition to clarify that the returned definition has no custom metadata properties.

• VectorStoreWriter.cs: Remove TKey generic parameter, hardcode Guid key type, simplify key generation to Guid.NewGuid(). Make class non-sealed and add virtual SetMetadata method (moved from IngestedChunkRecord) that throws NotSupportedException by default. Users who need metadata override this method in a derived VectorStoreWriter.

• VectorStoreWriterOptions.cs: Fix XML doc reference to match updated generic signature VectorStoreWriter{TChunk, TRecord}.

• Template and snapshot files: Simplify IngestedChunk to always use Guid key (removing #if conditionals), use EmbeddingStorageName for StorageName in derived types. Remove stale using System.Text.Json.Serialization from all snapshot files.

• Test files: Remove unnecessary JsonPropertyName attributes and using directives. Add CanWriteChunksWithCustomDefinition test proving users can create their own VectorStoreCollectionDefinition with custom storage names to map to pre-existing collections with different schemas. Add TestVectorStoreWriterWithMetadata to demonstrate metadata handling via a derived VectorStoreWriter. Update TestChunkRecordWithMetadata to no longer override SetMetadata (now handled by the writer).

• Fix unnecessary using System in TestChunkRecordWithMetadata.cs (CI failure S1128)

• Remove stale using System.Text.Json.Serialization from all 5 snapshot IngestedChunk.cs files (template snapshot test failures)

• Rename CreateCollectionDefinition to CreateDefaultCollectionDefinition

• Make VectorStoreWriter non-sealed and move SetMetadata from IngestedChunkRecord to VectorStoreWriter

• Build with -warnaserror — 0 warnings, 0 errors

• Run tests — 123 passed, 11 skipped, 0 failed

---

📍 Connect Copilot coding agent with Jira, Azure Boards or Linear to delegate work to Copilot in one click without leaving your project management tool.

[adamsitnik]

@copilot address my feedback

[adamsitnik]

@copilot please address my feedback

[Copilot]

@copilot please address my feedback

All four feedback items addressed in c3c57e0: storage name consts made private, VectorStoreWriterOptions doc fixed, [JsonPropertyName] removed from test, and new CanWriteChunksWithCustomDefinition test added.

[adamsitnik]

@copilot address my feedback

[adamsitnik]

@copilot please address my feedback

[adamsitnik]

@copilot the CI has failed with following error:

test\Libraries\Microsoft.Extensions.DataIngestion.Tests\Writers\TestChunkRecordWithMetadata.cs(4,1): error S1128: (NETCORE_ENGINEERING_TELEMETRY=Build) Remove this unnecessary 'using'. (https://rules.sonarsource.com/csharp/RSPEC-1128

Perform full build and fix all the warnings and errors, run the tests locally before pushing changes

[Copilot]

@copilot the CI has failed with following error:

Removed the unnecessary using System; from TestChunkRecordWithMetadata.cs in 64af799. Built with -warnaserror — 0 warnings, 0 errors. All 123 tests pass.

[Copilot]

@copilot there are 5 test failures in the Microsoft.Extensions.AI.Templates.Tests project. Some of these:

...

Removed stale using System.Text.Json.Serialization; from all 5 snapshot IngestedChunk.cs files in 81f4817. The template no longer uses JSON attributes, so the generated code doesn't include that using directive — but the verified snapshot files still had it, causing the mismatch. Build passes with 0 warnings/errors and all 123 DataIngestion tests pass.

[adamsitnik]

LGTM, @roji the PR is ready for review.

[adamsitnik]

To other reviewers: this is very important. My idea was following:

• introduce a non-sealed IngestedChunkRecord<TChunk> type that comes with all the default properties. It gives us the ability to perform the query to get chunks that point to the same document but also allows the users for an easy RAG (they don't need to provide their own type)
• those who don't need any custom schema, can just use this type and call CreateCollectionDefinition to create the definition (it's required, because they need so somehow provide the dimensionCount).
• those who need a custom schema, can create it and pass to VectoreStoreCollection ctor
• the other way to customize the schema is to derive from IngestedChunkRecord<TChunk> and override selected properties.

[roji]

OK... It has been a while since we discussed all this so I may have forgotten some considerations we talked about.

So If I understand correctly, this moves away from the previous dynamic approach (Dictionary<string, object?>) to a typed approach. Some comments:

• Importantly, typed mapping with MEVD is (currently) not trimming (and therefore NativeAOT)-compatible... Serializing/deserializing a dictionary is easy enough, but doing it with a .NET type requires a source generator which we don't yet have. I think that's going to be a problem.
• I admit it took me quite a while to understand how custom metadata is supposed to work (with SetMetadata()) and why; having a partly typed, partly dynamic story seems to introduce quite a bit of complexity/weirdness. Here's my understanding:
  • A user that wants custom metadata must extend IngestedChunkRecord (this already feels a bit heavy compared to just having a Dictionary<string, object?> as before).
  • On their CustomIngestedChunkRecord, they add .NET properties for the extra metadata.
  • But they must also override SetMetadata(), to copy the dynamic metadata properties from the incoming IngestionChunk to the strongly-typed .NET properties on CustomIngestedChunkRecord. That's some various boilerplate-y, tedious glue between the dynamic nature of IngestionChunk and the static nature of IngestedChunkRecord.
  • (aside from all the above, they must also call CreateCollectionDefinition() to get the VectorStoreCollectionDefinition, and mutate that to add their custom properties. But that's unrelated)
• Note that their code in SetMetadata() can get out of sync... Like I can see someone adding a new static property on CustomIngestedChunkRecord, but then forgetting to update SetMetadata() (and then we silently write empty properties).
  * In other words, users need to keep (1) the incoming IngestionChunk's metadata (wherever it's populated), (2) CustomIngestedChunkRecord's .NET properties, (3) SetMetadata() and (4) the record definition in sync, which seems quite brittle... With the previous, fully dynamic model only (1) and (4) needed to be kept in sync (which is the absolute minimum)
• Because of all this, I'm trying to understand the value we get out of this partly static/partly dynamic design, compared to simply continuing to map Dictionary<string, object?> as before, and whether it's worth it...

[adamsitnik]

To other reviewers: These settings are no longer configured by the writer, they are part of the collection creation process.

[adamsitnik]

To other reviewers: this is the simplest way of configuring the dimension count (overriding virtual property and annotating it with the right attribute)

[adamsitnik]

To other reviewers: this is an example of providing custom schema (without the need to provide a dedicated mapper)

[adamsitnik]

To other reviewers: I am going to work on removing this generic argument very soon (I want IngestionChunk to be able to represent any input without using generic argument). But it's out of the scope of this PR.

[roji]

I know you're going to remove the generic argument, but FYI the TChunk name is causing me a bit of confusion, also in IngestionChunk<TChunk> (as if it's a chunk over itself). When reading this code I wasn't sure if with IngestedChunkRecord<TChunk>, TChunk should be string or IngestionChunk<string>.

So maybe consider renaming TChunk to just T (or TContent) everywhere.

[adamsitnik]

Yes, I really want to remove it and now I think I even know how ( #7404)

Please keep in mind it's Preview2 branch, so whatever gets merged does not automatically gets released to nuget.org. So I would prefer to keep TChunk here and just remove it completely in next PR.

[roji]

Hey @adamsitnik here are some first thoughts/questions about the new design... We should probably get past these before me reviewing the rest of the PR in detail. Feels like maybe we should jump into a call to discuss this stuff.

[roji]

I know you're going to remove the generic argument, but FYI the TChunk name is causing me a bit of confusion, also in IngestionChunk<TChunk> (as if it's a chunk over itself). When reading this code I wasn't sure if with IngestedChunkRecord<TChunk>, TChunk should be string or IngestionChunk<string>.

So maybe consider renaming TChunk to just T (or TContent) everywhere.

[roji]

OK... It has been a while since we discussed all this so I may have forgotten some considerations we talked about.

So If I understand correctly, this moves away from the previous dynamic approach (Dictionary<string, object?>) to a typed approach. Some comments:

• Importantly, typed mapping with MEVD is (currently) not trimming (and therefore NativeAOT)-compatible... Serializing/deserializing a dictionary is easy enough, but doing it with a .NET type requires a source generator which we don't yet have. I think that's going to be a problem.
• I admit it took me quite a while to understand how custom metadata is supposed to work (with SetMetadata()) and why; having a partly typed, partly dynamic story seems to introduce quite a bit of complexity/weirdness. Here's my understanding:
  • A user that wants custom metadata must extend IngestedChunkRecord (this already feels a bit heavy compared to just having a Dictionary<string, object?> as before).
  • On their CustomIngestedChunkRecord, they add .NET properties for the extra metadata.
  • But they must also override SetMetadata(), to copy the dynamic metadata properties from the incoming IngestionChunk to the strongly-typed .NET properties on CustomIngestedChunkRecord. That's some various boilerplate-y, tedious glue between the dynamic nature of IngestionChunk and the static nature of IngestedChunkRecord.
  • (aside from all the above, they must also call CreateCollectionDefinition() to get the VectorStoreCollectionDefinition, and mutate that to add their custom properties. But that's unrelated)
• Note that their code in SetMetadata() can get out of sync... Like I can see someone adding a new static property on CustomIngestedChunkRecord, but then forgetting to update SetMetadata() (and then we silently write empty properties).
  * In other words, users need to keep (1) the incoming IngestionChunk's metadata (wherever it's populated), (2) CustomIngestedChunkRecord's .NET properties, (3) SetMetadata() and (4) the record definition in sync, which seems quite brittle... With the previous, fully dynamic model only (1) and (4) needed to be kept in sync (which is the absolute minimum)
• Because of all this, I'm trying to understand the value we get out of this partly static/partly dynamic design, compared to simply continuing to map Dictionary<string, object?> as before, and whether it's worth it...

[adamsitnik]

Because of all this, I'm trying to understand the value we get out of this partly static/partly dynamic design, compared to simply continuing to map Dictionary<string, object?> as before, and whether it's worth it...

So far, there was only one schema (created by us), so we knew how to do the mapping. If we continue to map to Dictionary<string, object?> but also accept a custom schema, the user needs to tell us how exactly we should perform the mapping.

And once they want to do RAG, they need to re-map the dictionary using these magic names.

With strongly typed approach, they don't need to. And RAG is much easier. But at a cost of complicated metadata story (which is not very common scenario).

Let's have a call later today and discuss it.

[roji]

If we continue to map to Dictionary<string, object?> but also accept a custom schema, the user needs to tell us how exactly we should perform the mapping.

Isn't that still the case even after this PR? The vector collection definition still needs to be tweaked by the user to include the custom metadata properties, and these must correspond 100% to what will actually be coming in on IngestionChunk, right?

With strongly typed approach, they don't need to. And RAG is much easier. But at a cost of complicated metadata story (which is not very common scenario).

Right, I can see that.

The basic problem here - and I think the source of the complexity - is that we have a dynamic metadata scheme on the ingestion framework side (on IngestionChunk), and we're trying to shoehorn that into a static, typed scheme for MEVD.

Maybe an alternative here is to say that the built-in VectorStoreWriter only works with the universal/built-in fields (no custom metadata), and if you want custom metadata you need to do your own VectorStoreWriter; this effectively moves the complexity (like SetMetadata()) from IngestedChunkRecord into the writer. We may be able to think of making VectorStoreWriter extensible with hooks for this (again, instead of having an extensible IngestedChunkRecord with its SetMetadata() hook).

Just some ideas I haven't fully thought through yet. Let's discuss.

[adamsitnik]

@copilot please address my feedback

[adamsitnik]

We discussed it offline and agreed I am going to remove it in a separate PR (#7410), as it most likely going to require a NuGet update and mirror sync.

[adamsitnik]

To other reviewers: This is an alternative to exposing entire schema. We can just expose a factory method that does the right thing.

The advantages;

• one method call instead of two
• clear message what needs to happen when you need NativeAOT.

The disadvantages:

• you need to know that it exists (other docs point to this method so I hope it won't be a big problem)
• using two generic arguments. In the near future ([MEDI] Make the IngestionChunk non-generic #7404) it will be only one.

[adamsitnik]

To other reviewers: So far, we were optimized for very easy ingestion. Now, the RAG is way simpler but when you need to use metadata, you need to create a derived type and handle it on your own. We throw here to avoid silent errors.