From c52fbc428eb6e6fcb5cf4b6bf2d48fd5e6a6713f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 19:29:38 +0000 Subject: [PATCH] fix(quartz): stop dropping a bird sighting's alt text, and stop allocating per role Two findings from an audit of this PR's own diff. BUG. The kind 2473 branch dropped the `alt` tag whenever commonName() parsed one out of it, on the reasoning that the alt is only Birdstar's boilerplate wrapper around the two species names. But commonName() matches a PREFIX and then cuts at the last " (", so a publisher can write anything after the parenthetical and it parses just the same: "Bird detection: Purple Gallinule (Porphyrio martinica) at Lake Merritt, 7am" yielded "Purple Gallinule" and the tail reached NO role, while indexableContent() still carried it. That is exactly the drift against the flat form this PR exists to remove, introduced by the PR itself. The alt now always reaches the summary tier: the duplicate it repeats there lands in the weakest role, whereas the drop cost recall outright. PERFORMANCE. Extraction runs once per stored event and per full reindex, and the funnel allocated a throwaway list per role whether or not the role had anything in it. The single-value tiers() overload wrapped each of its three values in a list only for cleanAll() to build another; cleanAll() allocated even when every value was null; the hashtag role called hashtags(), which allocates unconditionally, on every event including the great majority carrying no `t` tag; and locationValues() allocated a list per event to hold, almost always, nothing. Both overloads now end in one build() -- so hashtags and locations are still filled in a single place no branch can forget -- and each collector allocates lazily. Measured with getThreadAllocatedBytes over 1M extractions, JIT-warm: kind 1, no tags 160 -> 40 B/event kind 1, six tags 528 -> 168 B/event kind 30023 title+summary 272 -> 88 B/event The hash of every extracted value is unchanged across the A/B, and the guard added before hashtags() is HashtagTag.parse's own acceptance test, so it cannot skip a tag the accessor would have returned. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GznRZiv3zS7V9c2QQ9aMk9 --- .../nip50Search/SearchFieldExtractor.kt | 98 ++++++++++++++----- .../nip50Search/SearchFieldExtractorTest.kt | 42 ++++++-- 2 files changed, 111 insertions(+), 29 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt index b73326c311..97fcb8b7cb 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractor.kt @@ -44,7 +44,10 @@ import com.vitorpamplona.quartz.experimental.trustedLists.TrustedListEvent import com.vitorpamplona.quartz.experimental.zapPolls.ZapPollEvent import com.vitorpamplona.quartz.feedDefinition.FeedDefinitionEvent import com.vitorpamplona.quartz.nip01Core.core.Event +import com.vitorpamplona.quartz.nip01Core.core.fastAny +import com.vitorpamplona.quartz.nip01Core.core.fastForEach import com.vitorpamplona.quartz.nip01Core.metadata.MetadataEvent +import com.vitorpamplona.quartz.nip01Core.tags.hashtags.HashtagTag import com.vitorpamplona.quartz.nip01Core.tags.hashtags.hashtags import com.vitorpamplona.quartz.nip10Notes.TextNoteEvent import com.vitorpamplona.quartz.nip14Subject.subject @@ -494,15 +497,18 @@ object SearchFieldExtractor { // kind 2473 -- a sighting IS its species, under both names: the // scientific one from the `n` tag and the vernacular one - // commonName() parses out of the `alt`. Once that parse succeeds - // the alt is only Birdstar's boilerplate wrapper around the two - // ("Bird detection: ()"), so indexing it - // whole would repeat both names in a second role; the alt is - // carried only when it is NOT that shape and so holds text of its - // own. + // commonName() parses out of the `alt`. The `alt` itself still + // goes to the summary tier whole. It is usually just Birdstar's + // boilerplate wrapper around those two names ("Bird detection: + // ()"), so this repeats them in a second + // role -- but only commonName()'s PREFIX match decides that the + // alt is boilerplate, and a publisher can write anything after + // the parenthetical. Dropping the alt on a prefix match lost that + // tail from every role, which is precisely the drift against + // indexableContent() this file exists to prevent: the repeat + // costs a duplicate in the weakest role, the drop cost recall. is BirdDetectionEvent -> { - val common = event.commonName() - tiers(event, listOf(event.speciesName(), common), listOf(event.summary().takeIf { common == null }), null) + tiers(event, listOf(event.speciesName(), event.commonName()), listOf(event.summary()), null) } // kind 12473 is a life LIST, not a sighting: its species names are @@ -628,46 +634,94 @@ object SearchFieldExtractor { } } - /** Single-value convenience over the list funnel — most kinds carry one title, one summary, one body. */ + /** + * Single-value convenience over the list funnel — most kinds carry one + * title, one summary, one body. Cleans each value straight into its role + * rather than wrapping it in a list first: extraction runs once per stored + * event, and the wrappers were three throwaway lists on every one of them. + */ private fun tiers( event: Event, primary: String?, secondary: String?, text: String?, website: String? = null, - ) = tiers(event, listOf(primary), listOf(secondary), text, listOf(website)) + ) = build(event, cleanOne(primary), cleanOne(secondary), text, cleanOne(website)) - /** - * The one funnel every content branch uses — [IndexableFields.Tiered.hashtags] - * and [IndexableFields.Tiered.locations] are filled here, so no branch can - * forget them. Values stay UNJOINED: separator choices belong to the backend. - */ private fun tiers( event: Event, primary: List, secondary: List, text: String?, websites: List = emptyList(), + ) = build(event, cleanAll(primary), cleanAll(secondary), text, cleanAll(websites)) + + /** + * The one funnel BOTH [tiers] overloads end in — [IndexableFields.Tiered.hashtags] + * and [IndexableFields.Tiered.locations] are filled here, so no branch can + * forget them. Values stay UNJOINED: separator choices belong to the backend. + */ + private fun build( + event: Event, + primary: List, + secondary: List, + text: String?, + websites: List, ) = IndexableFields.Tiered( - primary = cleanAll(primary), - secondary = cleanAll(secondary), + primary = primary, + secondary = secondary, text = clean(text), - hashtags = cleanAll(event.tags.hashtags()), + hashtags = hashtagValues(event), locations = locationValues(event), - websites = cleanAll(websites), + websites = websites, ) /** Trim and drop empties at the single funnel every derived string passes through. */ private fun clean(s: String?): String? = s?.trim()?.ifEmpty { null } - private fun cleanAll(parts: List): List = parts.mapNotNull { clean(it) } + private fun cleanOne(s: String?): List = clean(s)?.let { listOf(it) } ?: emptyList() + + /** + * Collects lazily: a role whose values are all absent — the common case on + * most kinds — costs no list at all. + */ + private fun cleanAll(parts: List): List { + var values: MutableList? = null + for (i in parts.indices) { + val value = clean(parts[i]) ?: continue + (values ?: ArrayList(parts.size).also { values = it }).add(value) + } + return values ?: emptyList() + } + + /** + * The hashtag role. [hashtags] allocates unconditionally, so the scan for + * a `t` tag comes first — most events carry none. The guard is exactly + * [HashtagTag.parse]'s own acceptance test, so it can never skip a tag the + * accessor would have returned. + */ + private fun hashtagValues(event: Event): List = if (!event.tags.fastAny(HashtagTag::isTagged)) emptyList() else cleanAll(event.tags.hashtags()) /** * Every `location` tag value, on ANY kind. Deliberately a raw scan, not a * typed accessor: Quartz's LocationTag classes are per-NIP (calendar, * picture, classifieds) and only those kinds expose locations(), while * this funnel must also catch location tags on kinds whose class doesn't - * model them. + * model them. Collects lazily, like [cleanAll]: an event with no location + * tag — nearly all of them — allocates nothing here. */ - private fun locationValues(event: Event): List = event.tags.mapNotNull { tag -> if (tag.getOrNull(0) != "location") null else clean(tag.getOrNull(1)) } + private fun locationValues(event: Event): List { + var values: MutableList? = null + event.tags.fastForEach { tag -> + if (tag.size > 1 && tag[0] == LOCATION_TAG) { + val value = clean(tag[1]) + if (value != null) { + (values ?: ArrayList(2).also { values = it }).add(value) + } + } + } + return values ?: emptyList() + } + + private const val LOCATION_TAG = "location" } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt index 227b8b531c..3075095a39 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip50Search/SearchFieldExtractorTest.kt @@ -298,24 +298,52 @@ class SearchFieldExtractorTest { @Test fun birdSightingsAreNamedByTheirSpeciesUnderBothNames() { - // Birdstar's `alt` is a boilerplate wrapper around the two names, so - // once commonName() has parsed it there is nothing left in it to - // index -- carrying it whole would repeat both names in a second role. + // The scientific name comes from the `n` tag, the vernacular one from + // commonName()'s parse of the `alt` -- both are names, so both are + // titles. The alt still reaches the summary tier whole. val tags = arrayOf( arrayOf("n", "Porphyrio martinica"), arrayOf("alt", "Bird detection: Purple Gallinule (Porphyrio martinica)"), ) val fields = SearchFieldExtractor.extract(BirdDetectionEvent("26".repeat(32), alice, 1L, tags, "", "")) - assertEquals(IndexableFields.Tiered(primary = listOf("Porphyrio martinica", "Purple Gallinule")), fields) + assertEquals( + IndexableFields.Tiered( + primary = listOf("Porphyrio martinica", "Purple Gallinule"), + secondary = listOf("Bird detection: Purple Gallinule (Porphyrio martinica)"), + ), + fields, + ) + } + + @Test + fun aBirdSightingKeepsAltTextBeyondTheParsedNames() { + // commonName() only matches a PREFIX, so a publisher can write + // anything after the parenthetical. Dropping the alt whenever that + // prefix parsed lost the tail ("at Lake Merritt, 7am") from every + // role, while indexableContent() still carried it -- the exact drift + // this extractor exists to prevent. + val tags = + arrayOf( + arrayOf("n", "Porphyrio martinica"), + arrayOf("alt", "Bird detection: Purple Gallinule (Porphyrio martinica) at Lake Merritt, 7am"), + ) + val fields = SearchFieldExtractor.extract(BirdDetectionEvent("3a".repeat(32), alice, 1L, tags, "", "")) + assertEquals( + IndexableFields.Tiered( + primary = listOf("Porphyrio martinica", "Purple Gallinule"), + secondary = listOf("Bird detection: Purple Gallinule (Porphyrio martinica) at Lake Merritt, 7am"), + ), + fields, + ) } @Test fun aBirdSightingWithAnUnrecognizedAltStillIndexesIt() { - // A publisher that words the alt differently keeps it: the summary is - // dropped only when commonName() proves it was the boilerplate. + // An alt that does not start with the known prefix parses to no + // common name, and is carried as the summary it is. val tags = arrayOf(arrayOf("n", "Ramphastos toco"), arrayOf("alt", "a toucan at the feeder")) - val fields = SearchFieldExtractor.extract(BirdDetectionEvent("3a".repeat(32), alice, 1L, tags, "", "")) + val fields = SearchFieldExtractor.extract(BirdDetectionEvent("3c".repeat(32), alice, 1L, tags, "", "")) assertEquals( IndexableFields.Tiered(primary = listOf("Ramphastos toco"), secondary = listOf("a toucan at the feeder")), fields,