fix: postgres search query does not use index

[roldengarm]

Motivation and Context (Why the change? What's the scenario?)

I've imported about 6 million documents with PostgresDB as backend. I've created an HNSW index. When doing a search (using WebClient.Ask/Search) I keep getting timeouts. Upon further investigation, I saw that the query used in Kernel Memory is not optimisable as it's doing an inline calculation of "1 - the difference".

After I removed the "1 - " in our fork & deployed it, it was successfully using the index and I would get a response within 5 - 10 seconds.
I've used pgAdmin4 to confirm that the new query is using the index.

See also discussion with @dluc here: #663 (reply in thread)

[marcominerva]

I think that, as this PR changes the returned similarityActualValue from PostgresSQL, also the minSimilarity usage must be adjusted.

In fact, going deep in the code, I see this:

kernel-memory/extensions/Postgres/Postgres/Internals/PostgresDbClient.cs

Line 419 in 3d34260

sqlUserValues[similarityPlaceholder] = minSimilarity;

kernel-memory/extensions/Postgres/Postgres/Internals/PostgresDbClient.cs

Lines 432 to 448 in 3d34260

	cmd.CommandText = @$"
	SELECT {columns}, 1 - ({this._colEmbedding} <=> @embedding) AS {similarityActualValue}
	FROM {tableName}
	WHERE {filterSql}
	ORDER BY {similarityActualValue} DESC
	LIMIT @limit
	OFFSET @offset
	";
	cmd.Parameters.AddWithValue("@embedding", target);
	cmd.Parameters.AddWithValue("@limit", limit);
	cmd.Parameters.AddWithValue("@offset", offset);
	foreach (KeyValuePair<string, object> kv in sqlUserValues)
	{
	cmd.Parameters.AddWithValue(kv.Key, kv.Value);
	}

In particular, the foreach adds sqlUserValues (included similarityPlaceholder= "@__min_similarity") to query parameters, but this parameter seems not to be used.

Instead, records are filtered by similarity after the query execution:

kernel-memory/extensions/Postgres/Postgres/Internals/PostgresDbClient.cs

Lines 458 to 469 in 3d34260

	var run = true;
	while (run && await dataReader.ReadAsync(cancellationToken).ConfigureAwait(false))
	{
	double similarity = dataReader.GetDouble(dataReader.GetOrdinal(similarityActualValue));
	if (similarity < minSimilarity)
	{
	run = false;
	continue;
	}
	result.Add((this.ReadEntry(dataReader, withEmbeddings), similarity));
	}

[roldengarm]

Thanks for checking @marcominerva I think you're right. Perhaps it worked at my end as minSimilarity was 0, haven't checked. But yes, I guess the check would have to be flipped.

But, wouldn't it be better & easier to read if the minSimilarity check would be part of the query? Then it could also be optimised by Postgres, instead of having an in-memory search / filtering afterwards. Would need to check filterSql a bit further, as in my test case it was empty / TRUE.

[marcominerva]

But, wouldn't it be better & easier to read if the minSimilarity check would be part of the query? Then it could also be optimised by Postgres, instead of having an in-memory search / filtering afterwards. Would need to check filterSql a bit further, as in my test case it was empty / TRUE.

Yes, it is exactly what I mean when I said that minSimilarity parameter is currently not used in the query.

[roldengarm]

But, wouldn't it be better & easier to read if the minSimilarity check would be part of the query? Then it could also be optimised by Postgres, instead of having an in-memory search / filtering afterwards. Would need to check filterSql a bit further, as in my test case it was empty / TRUE.

Yes, it is exactly what I mean when I said that minSimilarity parameter is currently not used in the query.

Hi @marcominerva I've made the changes you recommended, the difference filter is now being done in the SQL query.
For some reason, I could not do "WHERE {colDifference} < {maxDifference}", that was causing a "table 'km-default'" can not be found.

The query still performs well, but on my large dataset I'm getting different results now. I'll investigate further as obviously the results should be similar. If you have a chance to review and see any obvious mistakes, that would be great.

[marcominerva]

What you have called difference is actually the distance (the closer it is to 0, the closer the vectors themselves are). I suggest you to rename the code in this way to better specify the intent.

Moreover, Kernel Memory works with the concept of relevance (the closer it is to 1, more relevant the vectors are). All memories return records with the most relevance first. See for example:

kernel-memory/service/Core/Search/SearchClient.cs

Lines 227 to 238 in 3d34260

	IAsyncEnumerable<(MemoryRecord, double)> matches = this._memoryDb.GetSimilarListAsync(
	index: index,
	text: question,
	filters: filters,
	minRelevance: minRelevance,
	limit: this._config.MaxMatchesCount,
	withEmbeddings: false,
	cancellationToken: cancellationToken);
	// Memories are sorted by relevance, starting from the most relevant
	await foreach ((MemoryRecord memory, double relevance) in matches.ConfigureAwait(false))
	{

Instead, now in you're code the result list is ordered in an ascending way based on the distance (even if you calculate the similarity, the list is created reading the records that are returned by the query). So, I think you need to reverse the result list.

[roldengarm]

What you have called difference is actually the distance (the closer it is to 0, the closer the vectors themselves are). I suggest you to rename the code in this way to better specify the intent.

Thanks, I've changed it to distance.

Instead, now in you're code the result list is ordered in an ascending way based on the distance (even if you calculate the similarity, the list is created reading the records that are returned by the query). So, I think you need to reverse the result list.

I have already reversed the ORDER BY from DESC (on similarity) to ASC (on distance), so I think that's correct. Upon further investigation it turns out the results were not different.
Can you please do a review and see if it can be merged? @marcominerva

[roldengarm]

@microsoft-github-policy-service agree [company="GenText"]

[roldengarm]

@microsoft-github-policy-service agree company="GenText"

[marcominerva]

I have already reversed the ORDER BY from DESC (on similarity) to ASC (on distance), so I think that's correct. Upon further investigation it turns out the results were not different. Can you please do a review and see if it can be merged? @marcominerva

I too have made a test with the old and the new approach and I have verified that now results are returned in the correct order. I would suggest only a last minor fix: rename the colDistance variable using __ as prefix, as in the original code:

string colDistance = "__distance";

Remember also to correct the comment at line 437 (you're still using colDifference).

After that, the code seems OK to me. Now we must wait for the approval by @dluc.

[dluc]

Suggested change

this._log.LogTrace("SQL: {0}", cmd.CommandText);

[roldengarm]

Fixed, thanks @dluc

[dluc]

could you grant maintainers edit on the PR?

[roldengarm]

could you grant maintainers edit on the PR?

Sorry @dluc , that option isn't there, I guess because it's not a user-owned fork, but an org owned? Unless I'm missing something, but I believe the option should appear here on the right at the bottom based on Github Docs.

[dluc]

Suggested change

	var maxDistance = 1 - minSimilarity;
	var maxDistance = 1 - minSimilarity;

[dluc]

nit, codestyle: missing empty line

[dluc]

could you grant maintainers edit on the PR?

Sorry @dluc , that option isn't there, I guess because it's not a user-owned fork, but an org owned? Unless I'm missing something, but I believe the option should appear here on the right at the bottom based on Github Docs.

please upvote https://github.com/orgs/community/discussions/5634