kaxil commented on PR #71478: URL: https://github.com/apache/airflow/pull/71478#issuecomment-5696639717
Approving this. I rebuilt the site at `81d4d470` with real dependencies and queried pagefind's own runtime rather than reasoning about the index, and the collision filter does what it set out to do: no incumbent loses its top spot (`openai` 11.30 vs common-ai 5.07, `anthropic` 12.15 vs 3.66, `google` 1.55 vs 0.46, `azure` 5.51 vs 1.75), while `ollama`, `groq`, `deepseek`, `vllm` and `mistral` went from zero results to Common AI. The category refactor came out clean: generating the category map at this head and on `main` over the same 107 providers gives zero differences, so routing both consumers through `categoryMatchesProvider` changed no existing assignment. I also checked the statement in the `exploreCategories.js` comment and it holds exactly, forcing the flag on for all seven categories changes only `cloud`. Four small things I'd take or leave, none of them worth holding this up: 1. The first bullet in the description says "Both search paths now match on the external services a provider's connections reach". That holds for 9 of the 11 services, but `AWS Bedrock` and `Google Vertex AI` are deliberately kept out of the pagefind text, so `bedrock` and `vertex` find Common AI on /providers/ and return nothing in Cmd-K. Both examples you give are accurate, it's the general sentence that could use the caveat, since it becomes the commit message. 2. `providerExternalServices.js` line 74 names my handle and cites numbers that only exist in a review thread, which nobody reading the file later can check. Could you drop the attribution? The mechanism reads fine on its own: single-token vendor names stay in the indexed text because the incumbent still ranks first on those queries. The clause after it isn't right either, since `openai`, `anthropic` and `google` are separate providers in this tree, so a single-token name would collide with them rather than with Common AI itself. 3. The `gcp` alias never fires. No service name or `integration-name` across the 107 `provider.yaml` files contains that token, and removing the entry leaves the filter output byte for byte identical, whereas removing `aws` stops "AWS Bedrock" being caught. The comment above it also points at "the collision-detection PR" for design notes, and this diff is that PR. 4. `tokenizeForCollisionCheck` splits on whitespace only, while all three `normalize()` copies and the matching rule the README now states collapse `-`, `_` and whitespace together. `external-services` is free-form, so `AWS-Bedrock` or `AWS_Bedrock` would slip past the check that exists to catch `AWS Bedrock`. I ran `/[-_\s]+/` there against all 107 files and the output is identical on today's data, so it's a safe swap whenever you next touch the file. Thanks for the README and comment updates from the last round, both went in cleanly. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
