Skip to content

Genomic colocation: add Short Variant, repair five Oracle-isms - #22

Open
jbrestel wants to merge 9 commits into
masterfrom
feat/variant-colocation
Open

Genomic colocation: add Short Variant, repair five Oracle-isms#22
jbrestel wants to merge 9 commits into
masterfrom
feat/variant-colocation

Conversation

@jbrestel

@jbrestel jbrestel commented Aug 15, 2026

Copy link
Copy Markdown
Member

What this is

Two things, and the second is bigger than the title suggests:

  1. Adds Short Variant support to the Genomic Colocation combine step.
  2. Repairs genomic colocation on PostgreSQL, which turned out to be entirely non-functional — not "broken for some record types". Four of the five defects below sit in code paths shared by every colocation, genes included.

Pairs with VEuPathDB/ApiCommonModel#220 — these must merge together, and this one should go first. The model PR adds a question that references plugin behaviour introduced here.

The five Oracle-isms

# Construct Location Broke
1 rownum getStandardSpanSql non-gene inputs
2 DECODE DynSpan branch genomic segments
3 regexp_substr returns text, and makeRegion does arithmetic on it DynSpan coordinates genomic segments
4 FROM (table_name) alias composeSql all colocation
5 getDefaultSchema() create/drop mismatch execute cleanup all colocation

Worth noting how each was found, because it says something about the test strategy: #1 and #2 by reading the code, #3 by code review, and #4 and #5 only by running it. Unit tests asserting contains() on generated SQL fragments passed throughout — they never composed the final join and never executed anything.

rownum is deleted, not ported. It filtered all rows to the feature_type of one arbitrary joined row — a defensive no-op even on Oracle (only 95 feature_source_id values out of 46M rows in apidb.FeatureLocation have more than one distinct feature_type) — and after this refactor it sits only on paths that are one-row-per-record by construction.

regexp_substr with Oracle's 4-arg signature is kept: it works on PostgreSQL 18 (select regexp_substr('chr1:100-200:r','[^:]+',1,3)r).

⚠️ Please review: which schema owns the span temp tables

This is the change I most want a second opinion on. The diff shows only schemanull, which hides a platform-semantics decision.

Oracle.getDefaultSchema(login)     → normalizeSchema(login)     // the login user's schema
PostgreSQL.getDefaultSchema(login) → normalizeSchema("public")  // hardcoded

getSpanSql issues an unqualified CREATE TABLE spanlogic<n>, which follows search_path"$user" on our appDbs. The cleanup dropped via getDefaultSchema(). Those coincide on Oracle and diverge on PostgreSQL, so tables were created in the login schema and the drop looked in public, raising table "spanlogic<n>" does not exist after the results were computed. A working colocation surfaced to the user as an error and leaked one table per run — 44 orphans were found in genomicsdb_071n.

Chosen fix: pass null, so dropTable emits a bare table name that resolves exactly as the CREATE did, on either platform.

Rejected alternatives, and why:

  • Qualify the CREATE with getDefaultSchema(), sending tables to public. Since PostgreSQL 15 the PUBLIC role has no CREATE on public by default, so this may fail outright on some deployments, and it puts per-request scratch tables in a shared schema.
  • Fix PostgreSQL.getDefaultSchema in FgpUtil to return the login schema. Arguably the real defect — the two implementations do not mean the same thing and callers cannot tell. Scoped out because that method is used across WDK and public may be correct for its other callers. If you prefer this, it should be its own change with a call-site audit.

The trade being made: scratch tables are a per-request implementation detail, so "wherever the login can write" is defensible — but the plugin now states nowhere which schema it writes to. Worth a follow-up ticket on the FgpUtil asymmetry either way.

The refactor

The record-class if/else in getSpanSql is replaced by a SpanSource interface plus a registry keyed by record-class full name:

Input record class Coordinate source is_top_level filter Strandless
TranscriptRecordClass apidb.FeatureLocation, joined on gene_source_id, feature_type='GeneFeature' yes no
DynSpanRecordClass parsed from the step's own cache table no — one row per record no
VariantRecordClass ApidbTuning.VariationAttributes no — one row per record yes

An unregistered record class now throws, naming the class, rather than silently falling back to a bare apidb.FeatureLocation join nobody validated for it.

Two invariants are documented in the code because they are easy to break:

  • Every SpanSource must alias its location table flmakeRegion hardcodes that prefix.
  • is_top_level = 1 must stay on the Transcript source. 2,503 rows in apidb.FeatureLocation have is_top_level = 0; they are human pseudoautosomal-region genes carrying duplicate chrX/chrY placements. Dropping the filter double-counts them. Do not "clean this up".

Flag.hasSnpFlag.strandless: same behaviour, named after the property rather than one record type. It suppresses the strand filter for the whole comparison, which is required — a variant is a point with is_reversed = 0, so "same strand" would otherwise silently return forward-strand genes only. Side effect worth knowing: the strand selector is inert for any comparison involving variants. Correct, but the UI still offers the control.

The unreachable SnpRecordClasses.SnpRecordClass branch is deleted — every SNP import is commented out of apiCommonModel.xml, so that record class cannot exist at runtime.

Test plan

  • mvn -pl WSFPlugin test — 59 tests, 0 failures, 2 pre-existing skips
  • Generated SQL executed read-only against genomicsdb_071n (PostgreSQL 18.4)
  • Gene → Gene, including a 10001 bp upstream offset
  • Gene → Variant
  • Variant → Gene
  • Variant → Genomic Span (exercises Variant-as-input and DynSpanSource, i.e. the DECODE port and the numeric cast)
  • Strand selector on a variant comparison returns results rather than zeroing out
  • UniDB not tested. RecordsBySpanLogic declares wsColumn project_id unconditionally while VariantRecordClass excludes project_id from its PK on UniDB. DynSpanRecordClass has the same shape and shares the query, so this is pre-existing — but DynSpan colocation has never worked on PostgreSQL, so nobody has exercised it. The shared query is deliberately untouched.

🤖 Generated with Claude Code

jbrestel and others added 9 commits August 15, 2026 12:10
An unregistered record class now throws rather than silently falling back
to a bare apidb.FeatureLocation join that was never validated for it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Also drops the synthetic is_top_level/feature_type columns, which existed
only to satisfy filters the one-row-per-record builder no longer applies.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
regexp_substr returns text, and makeRegion does arithmetic on start_min/
end_max. Oracle coerced implicitly; PostgreSQL raises 'operator does not
exist: text + integer', so segment colocation failed even after the DECODE
fix. Text comparison would also have ordered spans lexicographically.

Also: make the fl-alias assertion match the FROM clause instead of any
occurrence, lowercase the Oracle-ism checks, and guard spanSourceFor(null).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…paths

- getSpanSql looks up a SpanSource instead of branching on record-class name
- Flag.hasSnp -> Flag.strandless, named after the property not one record type
- deletes the unreachable SnpRecordClass branch: every SNP import is commented
  out of apiCommonModel.xml, so that record class cannot exist at runtime
- deletes the rownum subquery rather than porting it
Oracle accepts "FROM (table_name) alias"; PostgreSQL raises
'syntax error at or near ")"'. composeSql is shared by every colocation
regardless of record type, so this broke ALL of them on Postgres --
including gene <-> gene, which the earlier analysis had assumed working.

Found by live QA, not by the unit tests: they asserted on the per-source
CREATE TABLE statements and never touched the join composed from them.
composeSql is now package-private with a regression test.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
getDefaultSchema() means different things per platform: Oracle returns the
login user's schema -- exactly where an unqualified CREATE TABLE lands, so
create and drop agreed. PostgreSQL hardcodes "public", while an unqualified
CREATE follows search_path ("$user").

So the temp tables were created in the login schema and the drop looked in
public, failing with 'table "spanlogic<n>" does not exist' AFTER the results
were computed. A working colocation surfaced to the user as an error, and
leaked one table per run (38 found in the appDb).

Passing null makes dropTable emit a bare table name, resolving the same way
the CREATE did, on either platform.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Where a record type's genomic coordinates come from. One implementation per record
* class that may be an input to colocation.
*
* Implementations MUST alias their location table "fl" -- makeRegion() hardcodes that

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in what class is makeRegion()? i can't find any reference to it, either being defined or called

@steve-fischer-200 steve-fischer-200 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't understand all the commentary about 'fl' and makeRegion()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants