I've changed the argument from string to VectorStoreRecordPropertyModel just to make sure all the derived types deal with StorageName in explicit way (they know this value is always escaped, while some random string may or may not be escaped).

Good idea.

Are you sure unescaping is actually needed here (and in fact, is correct)? Escaping should only be needed when the column name is integrated in SQL (i.e. in a larger string containing a query), but this is an API specifically accepting the column name (not a SQL string). I don't think this is right (in fact, if the coiumn actually contains a double-column, this would cause a failure).

Regardless, for future improvement, a better approach here would simply to read back columns from NpgsqlDataReader based on their position, instead of based on their name - we're the ones generating the SQL that selects the columns, so we should know the order in which they were sent and can read it back... This is a bit more efficient and cleaner, etc.

It is needed, when I revert the change, the tests start to fail:

    Microsoft.Extensions.VectorData.VectorStoreOperationException : Call to vector store failed.
    ---- System.IndexOutOfRangeException : Field not found in row: embed""ding

In general, I've extended the SimpleModelFixture storage names with all kind of weird characters. Because of that, we have coverage for collection, single record and batch CRUD. Initially I've quoted only the obvious things, but after studying the failing tests I was able to find all the weird and unexpected places.

a better approach here would simply to read back columns from NpgsqlDataReader based on their position

I agree, but it would require more non-trivial changes (build a dictionary name-position for every query), as the order may vary (we don't always include vector fields etc). If that is not a problem, I would prefer not to do it in this PR.

It is needed, when I revert the change, the tests start to fail:

I see, that's because you've changed model building so that PropertyModel.StorageName is escaped - so now it has to be un-escaped (code):

property.StorageName = this.Options.EscapeIdentifier is not null
    ? this.Options.EscapeIdentifier(storageName)
    : storageName;

I'm not sure I'm a fan of the approach... I think the model storage name itself shouldn't be escaped, and then we apply escaping to it when needed, and don't apply when not needed (like here, when interacting with DbDataReader). I guess you're trying to defensively ensure that identifiers are escaped by default (in case we forget to do it somewhere); but remember that this isn't a security problem - both property names and the names of captured variables are part of the application code, and so are trusted (any security concerns are around the contents of captured variables, since that's what typically comes from untrusted user inputs).

So if we forget to escape an identifier somewhere, that's just a bug we should fix, and since we have some contexts where we want unescaped names (DbDataReader.GetOrdinal(), possibly logging....), I think the model should probably just contain the raw, unescaped name.

In any case, I agree that reading back columns based on their position is something that belong out of this PR and can be done later.

One more thought on this...

IIUC the property's StorageName is escaped, but it isn't quoted... That means that wherever the name needs to be integrated in SQL, we still need to remember to quote it - which I think removes the "safety-by-default" that having the escaped name in the model was supposed to provide in the first place...

To go all the way with that approach, we'd have the model storage name be both escaped and quoted; we'd then need to unquote and unescape for certain contexts (DbDataReader.GetOrdinal(), combining table/column names into an index name).

Or we go the other way, and just keep the raw, unquoted and unescaped name in the model, and simply call DelimitIdentifier wherever needed (again, we currently need to do this anyway for quoting). Building the index name would just be Delimit($"{tableName}_{columnName}") or whatever.

BTW do we also need to go over other non-SQL connectors to make sure they're safe etc.?

Shay Rojansky (@roji) I went through the non-SQL connectors (except of cosmos) and this was the only line of code I needed to change to get the tests passing. We are usually using strongly typed .NET APIs for these and/or JSON serializer that does all the escaping for us.

I'm still not sure why we're always removing the name... It's really useful for reading the SQL, 99% of the cases aren't going to be problematic, and the 1% (where there are weird chars) can be handled separately, no?

it was not being used

Was there a bug here that this fixes? Or is this about slightly better perf?

The input value (propertyName) can be column name with a character that is not valid for parameter names:

The tests I've provided cover it (when I revert this change, they fail)

So this is the same problem discussed above for parameter names inside the filter translator (i.e. TranslateQueryParameter).

For one thing, whatever logic we choose, we should probably not have two different implementations for handling parameter names (once in the filter translator, one here).

BTW if I'm very pedantic, just appending an index at the end isn't a 100% solution, since two parameters could still conflict (imagine having one parameter x that becomes x12 (because it's the 12th parameter), and x1 that also becomes x12 (because it's the second parameter). To fully ensure uniquenes, we need to simply record the parameters that have already been added in some set, and check if the new parameter is already in the set. This would also allow us only append the numbers when needed, rather than always appending them as the code currently does.

But I don't want us to get too bogged down in this. I'm also OK with merging for now and then revisiting later.

nit: we can use using static to not have to do SqliteConstants every time, e.g.:

BTW it's slightly odd for me that we're caching the escaped names (this code isn't that high-perf). But if we do want to do that, may as well also add the quotes around it?

In general, rather than treating escaping and quoting as two different concerns, we can have just a single API that accepts a table or column name and outputs the escaped, quoted string representation (in EF we call this DelimitIdentifier).

BTW it's slightly odd for me that we're caching the escaped names (this code isn't that high-perf).

The goal I had here was to ensure nobody is using unescaped values anywhere (doing it once was just perf side effect).

In general, rather than treating escaping and quoting as two different concerns, we can have just a single API that accepts a table or column name and outputs the escaped

This is a great idea, however I am afraid we need both quoted and not:

• for creating index names, we use table name and column name, if they were only quoted it would not work:
  

  semantic-kernel/dotnet/src/Connectors/Connectors.Memory.Sqlite/SqliteVectorStoreCollectionCommandBuilder.cs

  Line 57 in 2ec7d24

  builder.AppendLine($"CREATE INDEX {(ifNotExists ? "IF NOT EXISTS " : string.Empty)}\"{tableName}_{column.Name}_index\" ON \"{tableName}\"(\"{column.Name}\");");

• the SQLite vector extension does not support quoting for column names ;(

for creating index names, we use table name and column name, if they were only quoted it would not work:

For this, we can interpolate the completely raw (unquoted and unescaped) table and column names, and then run Delimit on the result. This way, we always start from raw identifiers and perform both escaping and quoting at the very end, after concatenation etc.

the SQLite vector extension does not support quoting for column names

Ah, that is very shitty indeed... But it's a very special exception (that will hopefully be fixed), so I wouldn't design our code around this one flaw (we can specifically do escaping without quoting at that one place for now).

The goal I had here was to ensure nobody is using unescaped values anywhere (doing it once was just perf side effect).

OK I think I missed this before.

See https://github.com/microsoft/semantic-kernel/pull/11676/files#r2064348530 for some thoughts on this.

The vec extension simply does not support quoting. Example:

CREATE VIRTUAL TABLE "vec_6a64a46d-6f95-4c8b-a8ad-4c4b009f0a68" USING vec0(
"id" TEXT PRIMARY KEY,
"embedding" FLOAT[3] distance_metric=cosine
);

SqliteException: SQLite Error 1: 'vec0 constructor error: Could not parse '"id" TEXT PRIMARY KEY''.