On Wed, 2 Sept 2026 at 11:16, David Geier <[email protected]> wrote: > > Hi! > > > Thanks for this—looks like a good improvement. > > Thanks for reviewing the patch. Attached is v9 with all reviewing > comments from below addressed.
Please add commit descriptions; the current patches don't have much other than their subjects which don't describe any rationale for the changes applied. 0001: LGTM 0002: 1. radix_sort_trigrams_signed has an `int count` argument, but the caller trigram_qsort uses size_t. 2. This increases the memory requirements of trigram_qsort by a huge margin. Could you change the radixsort to operate in-place, so that the new buffer is not needed? 3. The implementation for trigram_qsort_unsigned has not been adjusted nor replaced, and so keeps the same old performance that trigram_qsort_signed had. Please make sure to also adjust that implementation. 0003: 1. ginInsertBAEntry leaks the copied key Datum if the type is by-ref and already present in the accumulator. I think specifying HASH_KEYCOPY to only copying the Datum key if an entry is already present is more appropriate. 2. The comment for ginInsertBAEntries was completely removed, rather than updated to the new workings. Kind regards, Matthias van de Meent Databricks (https://www.databricks.com)
