Rework prerelease limit SQL

[cstamas]

Optimize the ranking SQL

[gnugomez]

looks good to me, although I'm not that far into jooq, I had to search for a lot of things you're doing here 😅

[Copilot]

Pull request overview

This PR reworks the SQL used to cap pre-release versions per extension by switching from a correlated subquery approach to a window-function-based ranking, then filtering by rank.

Changes:

• Introduces a dense_rank() window function and (optionally) wraps the base query in a CTE to filter pre-releases by rank.
• Switches the “minimal” row mapping for this query to use explicit column aliases plus a new RankedFieldMapper.
• Refactors FieldMapper implementations (including renaming the default mapper to IdentityFieldMapper) to support the new query shape.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[netomi]

Code review

🔴 Confirmed bug: SQL error on the default (uncapped) config

ExtensionVersionJooqRepository.findAllActiveByExtensionIdAndTargetPlatform (around line 139) appends

.and(field(name("extension_version_target_platform")).eq(targetPlatform))

unconditionally, but that alias only exists in the ranked_extension_version CTE-wrapped query shape used when maxPreReleaseVersions >= 0. When the cap is disabled (maxPreReleaseVersions < 0, which is the default — ovsx.extension-query.max-pre-release-versions=-1) and a target platform is supplied, this condition is applied to the plain (non-CTE) query, where the alias doesn't exist.

Reproduced directly against a real Postgres instance: findAllActiveByExtensionIdAndTargetPlatform(ids, "linux-x64", -1) throws

org.springframework.jdbc.BadSqlGrammarException / PSQLException: column "extension_version_target_platform" does not exist

Since VS Code's extensionQuery routinely supplies a target platform, essentially every such request hits this under the default production config and 500s. The existing tests never caught it because they only pair a non-null target platform with a cap of 100 (the CTE branch).

🟡 Plausible bug: denseRank() has no NULLS handling

denseRank() orders by semver_major/minor/patch DESC with no explicit NULLS LAST. Postgres defaults DESC to NULLS FIRST, so rows with unparseable/legacy version strings (semver columns NULL, still permitted by the nullable column) rank above every real semver version — instead of being excluded from ranking the way the old row-comparison-based subquery effectively excluded them. Combined with a small maxPreReleaseVersions cap, this could push the true latest pre-release out of the kept set, contradicting the method's own Javadoc guarantee ("the true latest pre-release — rank #1 — is never dropped by the cap"). Not covered by an existing test.

Efficiency

• denseRank() is computed unconditionally, before the cap check — so on the (default) uncapped path, the window function still runs and its column is produced for every row on this hot path (extensionQuery), for a value nothing ever reads.
• partitionBy(EXTENSION_ID, PRE_RELEASE) ranks the stable-release partition too, but the filter only ever reads rank for the pre-release partition — the stable-release partition's rank is computed and discarded, at a cost proportional to the (typically much larger) stable-version count.
• The capped branch wraps the whole prior query in a WITH ... AS (...) SELECT * FROM ... WHERE ... CTE just to reference the window-function alias in a WHERE — this adds a materialization boundary that can block predicate pushdown, versus scoping a subquery to pre-release rows only.

Readability / maintainability

• The query variable is declared as a plain SelectConditionStep, then conditionally reassigned to an entirely different, incompatible statement shape (the CTE wrapper, addressed via untyped field(name(...)) lookups) under the same identifier — this is exactly what let the confirmed bug above slip through; naming the two states differently (or extracting the CTE-wrapping into its own method) would make the shapes visible instead of hidden behind one reused variable.
• TableFieldMapper's remap guard (tableField.getTable() != table) is true for every field passed at all three call sites, so it's unreachable dead code — it currently "works" only because jOOQ's Record field-resolution has an undocumented alias-lineage fallback. A future latest-wrapped query whose read-side field isn't the literal same static Field singleton used when building the select could silently get the wrong value or null with no compiler error.
• RankedFieldMapper re-derives each CTE column name via string surgery on Field.getQualifiedName() rather than reusing the Fields the .as(...) calls already produced — this duplicates the naming convention in two unsynchronized places; a future codegen/config change or alias rename could break every read through this mapper with no compile-time signal.
• Line ~140 mixes field-lookup styles: two lines above use the typed rankedExtensionVersion.field("...", Boolean.class), this one builds an untyped, unqualified reference from scratch and (per the confirmed bug) is applied to the wrong query shape when uncapped.

---

🤖 Generated with Claude Code

[netomi]

Follow-up: regression test + performance test for the fixes above

Pushed two more commits addressing the issues from the review:

916acb84 — fixes the confirmed SQL bug and the NULLS-ordering issue

• findAllActiveByExtensionIdAndTargetPlatform now applies the target-platform filter correctly for both branches: the typed CTE field lookup when capped, and the real EXTENSION_VERSION.TARGET_PLATFORM column directly when uncapped (no CTE exists in that branch). Added targetPlatformFilterWorksWithTheCapDisabled, which reproduces the exact call shape that used to throw PSQLException: column "extension_version_target_platform" does not exist and now passes.
• Added .nullsLast() to the denseRank() semver ordering, so a legacy row with no parsed semver can no longer outrank a real semver version and get treated as the "latest" pre-release.

e2e5a642 — performance test comparing the new query against the one it replaces

Added newRankingApproachIsNotSlowerThanTheOldCorrelatedSubqueryApproach, since the point of this rework is performance and that deserved more than eyeballing the SQL. It ports the pre-rework ranking logic (a correlated COUNT(*) subquery per pre-release row — O(k²) for an extension with k pre-releases) as a test-local helper purely to have something to measure the new denseRank() window-function query against; the original method is gone from production code, only reconstructed here for comparison.

Seeds 3000 pre-release versions on one extension (the volume this method's own doc comment is written around — "one build per commit... thousands of entries"), runs one untimed warm-up of each query shape first, then times both against the real Testcontainers Postgres instance:

old (correlated subquery) took 3023ms, new (window function) took 56ms, for 3000 pre-release versions

~54x faster, not just "comparable." The assertion itself is intentionally loose (new ≤ old × 3 + 500ms) since this is real wall-clock timing against a containerized DB rather than a controlled microbenchmark — tight enough to catch a genuine regression, loose enough not to flake on CI noise. The measured numbers are logged on every run (logger.info(...)) so they stay visible going forward rather than only appearing once here.

Full suite: 1063/1063 passing.

---

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

[cstamas]

Coolio

[netomi]

Follow-up: extended the performance test with a third baseline

Pushed fb5f9bf6, extending the earlier performance test with a comparison against the method as it existed before pre-release limiting existed at all (pre-#2062) — a bare filtered SELECT with no ranking whatsoever.

Reasoning: the reworked query computes its denseRank() window function unconditionally, even on the uncapped (default, max-pre-release-versions=-1) path where nothing reads it. That's the path every request takes unless an operator opts into the cap, so it's worth knowing how it compares to the simplest possible query answering the same question.

Measured (3000 pre-release versions, same run, same margin-based assertions as before — not a tight ratio):

comparison	old/baseline	new
capped (the original comparison)	1620ms (correlated subquery)	20ms (window function) — ~80x faster
uncapped (new)	9ms (no ranking at all, pre-#2062)	132ms (unused window function still computed) — ~14x slower

So: the capped path is a clear, large win, as expected. The uncapped path shows a real, measurable cost from computing the window function even when it's discarded — this matches the efficiency note from the earlier review (denseRank() computed before the cap check). Still comfortably fast in absolute terms (132ms) and well within the test's margin, but worth having on record rather than only asserted away. Happy to move the denseRank() computation behind the cap check if you'd like the uncapped path to skip it entirely — let me know.

Full suite: 1064/1064 passing.

---

🤖 Generated with Claude Code

[netomi]

Follow-up: fixed the uncapped-path regression

Pushed 8726c1c0, chasing down and fixing the ~14x gap the extended performance test found (previous comment). Turned out there were two separate causes, and only one of them was what it looked like at first:

1. Skipped denseRank() entirely when the cap is disabled. Was computed unconditionally even though nothing read it on the uncapped (default) path.

2. That alone didn't move the needle — measured no change (still ~130ms vs ~19ms baseline). Dug further and found the actual dominant cost: RankedFieldMapper resolved each mapped column by constructing a brand-new, unrelated Field object and handing it to row.get(...) — which forces jOOQ into a slow fallback lookup instead of a fast indexed one. Resolving through the row's own field(String) lookup instead fixed it outright.

Once both were in, it became clear the uncapped path didn't need the aliasing/CTE machinery (or RankedFieldMapper) at all — that whole mechanism exists only so the capped path's CTE can reference columns by unique string name (NAMESPACE.ID/EXTENSION.ID/EXTENSION_VERSION.ID would otherwise all render as "id"). So the uncapped branch now just selects the plain, unaliased columns (like the pre-#2062 method did) and maps them via the existing toExtensionVersion/IdentityFieldMapper path, with no translation layer in the way at all.

Progression (3000 pre-release versions, same dataset each time):

step	no-limit baseline	new uncapped
before	~19ms	~127ms (~7x)
after skipping denseRank()	~19ms	~132ms (no change)
after fixing RankedFieldMapper's lookup	~19ms	~25-47ms
after splitting the field lists (this commit)	~32ms	~39ms

Also made toExtensionVersion package-private so the test can build a fair, equally-mapped baseline instead of a mapping-free one that would have understated the real per-row cost.

Full suite: 1064/1064 passing.

---

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

[netomi]

I think this PR is now ready for merging.

The old uncapped version is restored to not have any regression, the new capped version is now comparable in terms to speed and added some tests to compare the new version with the baseline.