Skip to content

Load the RF2 file types that have no table, and stop building empty schema qualifiers - #74

Open
dionmcm wants to merge 2 commits into
IHTSDO:developfrom
aehrc:upstream/mysql-fixes
Open

Load the RF2 file types that have no table, and stop building empty schema qualifiers#74
dionmcm wants to merge 2 commits into
IHTSDO:developfrom
aehrc:upstream/mysql-fixes

Conversation

@dionmcm

@dionmcm dionmcm commented Aug 31, 2026

Copy link
Copy Markdown

This can be ignored if transitioning to DuckDB, it is already fixed in that PR

Four MySQL-engine defects, each with a test that fails without the fix. No DuckDB involvement.

Reviewed on our fork first: aehrc#23

1. Four RF2 file types are never loaded

sct2_Identifier, der2_ccRefset_ExtendedAssociation and der2_csRefset_AttributeValueMap match no pattern in RF2FileTableMapper, so getLegacyTableName returns null and the files are skipped without a report entry. 9 files of a 66-file edition.

2. Integer and string SimpleMap share a table

SIMPLE_MAP_FILE_HEADER was der2_.*Refset_.*SimpleMap, matching both der2_sRefset_ and der2_iRefset_. Split, with der2_iRefset_ mapped to new isimplemaprefset_* tables. 3 more files on the same edition.

create-tables-mysql.sql gains the 12 tables these route to (4 types x delta/snapshot/full), each with a matching drop.

Checked: every added pattern is justified by a file in a real release, and no filename matches two patterns (replayed all 90 patterns against all 66 filenames). An earlier revision also added generic der2_cRefset_, der2_ccsRefset_ and der2_cciRefset_ patterns; they matched no file uniquely and shadowed 15 that had specific patterns, so they are not here.

3. getRvfSchemaMapping produces .langrefset_<SNAPSHOT>

An unprefixed token reached the release-type suffix block with rvfSchema still "", giving a leading dot. That cannot parse, and it defeats the isEmpty() guard below by making rvfSchema non-empty.

Five corpus scripts name a table this way; four are manifest-declared:

script token
mrcm-attribute-range-refset-validate-concept-ids mrcmattributerangerefset_s
mrcm-domain-refset-validate-concept-ids mrcmdomainrefset_s
changes-are-in-exepected-modules-validation-edition-proc moduledependencyrefset_d
changes-are-in-exepected-modules-validation-extension-proc moduledependencyrefset_d

Unqualified was already correct: the connection sets its default catalog to the prospective version.

The same block appended a suffix to an already-qualified <TEMP> token, turning v_attributedescription_s into <TEMP>.attributedescription_s.attributedescription_<SNAPSHOT>. No corpus script uses that form, so it is latent.

Stored-procedure detection also missed mixed-case CREATE PROCEDURE and all CREATE FUNCTION. Now case-insensitive, with CREATE FUNCTION recognised.

getRvfSchemaMapping is package-private so the unit test can reach it.

4. A second ENGINE clause

scripts/resource/res-table-edited-concept.sql ends ... ENGINE=MEMORY and is manifest-declared. The append guard excluded only the like/as forms, so the statement became ... ENGINE=MEMORY ENGINE = MyISAM, failed, and res_concepts_edited was never created — failing every assertion that selects from it. The condition is now a package-private predicate with a test using that statement verbatim.

Verification

38 tests pass. Each fix fails its test when reverted:

mapper reverted:   expected <isimplemaprefset_d> but was <simplemaprefset_d>
guard neutralised: expected <<TEMP>.attributedescription_s>
                   but was <<TEMP>.attributedescription_s.attributedescription_<SNAPSHOT>>

Full suite against a real MySQL 8.4.6, fresh rvf_master before each run:

clean upstream-develop   213 run, 1 failure,  0 errors, 21 skipped
with this PR             224 run, 0 failures, 0 errors, 21 skipped

The develop failure is AssertionGroupImporterTest.testAssertionsImport, "Total group must be equal expected: <49> but was: <50>". It counts assertion_group rows in a shared database that an earlier test in the suite has already populated; it passes when run alone. This PR shows 0 failures, but it adds tests and therefore changes execution order, so that is not attributable to the change. Flagging it because TestMySQLContainer sets withReuse(true) with a fixed port binding, so the same state can carry between runs with Docker too.

No Docker daemon is available on this host, so IntegrationTest was pointed at a real MySQL listening on the port TestMySQLContainer binds (50848, root/root, rvf_master). That patch was local to the measurement and is not part of this PR.

Not included

ColumnPatternTesterTest additions pin existing behaviour (a trailing tab delimiting an empty final column is not trailing whitespace) rather than fixing anything.

ExecutionCommand's text -> longtext is omitted: #70 already makes that change, byte for byte. We hit the same defect independently.

…chema qualifiers

Three independent defects on the MySQL engine, each with a test that fails
without its fix.

1. Four RF2 file types cannot be loaded at all.

`sct2_Identifier`, `der2_ccRefset_ExtendedAssociation` and
`der2_csRefset_AttributeValueMap` match no pattern in RF2FileTableMapper, so
getLegacyTableName returns null and the files are silently not loaded. Measured
on a real 66-file edition: 9 of its files are in this position. They are not
validated and nothing says so.

2. Integer and string SimpleMap files share one table.

SIMPLE_MAP_FILE_HEADER was `der2_.*Refset_.*SimpleMap`, which matches both
`der2_sRefset_SimpleMap` and `der2_iRefset_SimpleMap`, so both were loaded into
simplemaprefset_*. Split into `der2_sRefset_` and a new `der2_iRefset_` mapped to
isimplemaprefset_*; on the same edition that is 3 more files, previously in the
wrong table.

create-tables-mysql.sql gains the 12 tables (4 types x delta/snapshot/full) these
two fixes route to, each with the drop statement its neighbours have. Every added
pattern is justified by a file present in a real release, and no filename matches
two patterns - checked by replaying all 90 patterns against all 66 filenames.

3. getRvfSchemaMapping builds a qualifier with an empty schema.

An unprefixed token still reached the release-type suffix block, and since
rvfSchema was "" the result was a leading dot:

    langrefset_s  ->  "" + "." + "langrefset" + "_<SNAPSHOT>"  ->  .langrefset_<SNAPSHOT>

which cannot parse wherever it is substituted, and which defeats the isEmpty()
guard below by making rvfSchema non-empty. Five scripts in the pinned corpus name
a table this way and four are manifest-declared, so RVF runs them: both MRCM
refset concept-id checks and both changes-are-in-expected-modules procedures.
Unqualified was already correct, because the connection sets its default catalog
to the prospective version.

The same block also appended a suffix to a <TEMP> token that had already been
fully qualified, turning `v_attributedescription_s` into
`<TEMP>.attributedescription_s.attributedescription_<SNAPSHOT>`. No corpus script
uses that form today, so this one is latent rather than observed, but it is the
same defect and the guard fixes both.

Stored-procedure detection also missed mixed-case `CREATE PROCEDURE` and every
`CREATE FUNCTION`, because it compared against an exact-case and an all-lowercase
literal. Now compared case-insensitively, and CREATE FUNCTION is recognised.

getRvfSchemaMapping is package-private so the new unit test can reach it: every
whitespace token of every assertion passes through it, and its edge cases are
worth testing directly rather than through a database.

38 tests pass. Reverting any fix fails its test - the mapper test reports
`expected: <isimplemaprefset_d> but was: <simplemaprefset_d>`, and the importer
tests report the leading-dot rewrite and the doubled <TEMP> qualifier.
scripts/resource/res-table-edited-concept.sql creates its table as
'... key idx_conceptid (conceptid)) ENGINE=MEMORY', and it is manifest-declared,
so RVF runs it on every validation. The append guard only excluded the 'like' and
'as' forms, so this became

  create table if not exists res_concepts_edited(...) ENGINE=MEMORY ENGINE = MyISAM

which is a syntax error. The statement fails, res_concepts_edited is never
created, and every assertion that selects from it fails after it.

The condition is a string predicate, so it is now a package-private static method
tested as one - including the verbatim statement from that script, mixed-case
'Engine =', and the pre-existing like/as exclusions.
dionmcm added a commit to aehrc/release-validation-framework that referenced this pull request Sep 1, 2026
Same jar twice, one MySQL, one release, only rvf.execution.engine differing.
That is a stronger check than the existing nightly A/B, which compares a
separate Python engine against a production report - different corpus, host
and release, so a divergence there has several possible causes. Here the
engine is the only variable.

ci/engine_ab.py submits to both instances and gates; ci/engine_ab_stack.sh
brings up MySQL and both instances so the pipeline is a thin caller and the
tested thing is the same code a developer runs; az/azure-pipeline.engine-ab.yml
supplies inputs and publishes results. MySQL is the generic Linux tarball run
unprivileged, so no Docker daemon and no sudo on the agent.

First run, AU edition with a previous release, 8 cores:

    assertions joined on uuid   149
    identical failureCount      147  (98.7%)
    divergent                     2
    RVF/MySQL 1380s   DuckDB 120s   11.5x

And it earned its keep immediately: one of the two divergences is a
regression this repo introduced.

file-centric-snapshot-inactivated-component-module calls
validate_inactivated_component_module, whose cursor is "select table_name
from information_schema.tables ... and table_name like '%\\_d'" - every delta
table in the schema - and the dynamic SQL selects t1.id. identifier_d has no
id column; RF2 identifies those rows by alternateidentifier. So MySQL raises
"Unknown column t1.id in field list" and abandons the assertion, validating
nothing.

identifier_d exists in MySQL only because create-tables-mysql.sql now creates
it: one of the 12 tables added by the RF2-file-types change, fork PR #23,
already raised as IHTSDO IHTSDO#74. The DuckDB engine is unaffected because
publish_store.py unrolls the procedure at publish time and enumerated the 52
tables that actually have an id.

The second divergence is the known one: MySqlQueryTransformer drops a
statement naming a release the run does not hold, so the assertion is
reported PASSED for work never attempted, while DuckBinder reports it not
executed. DuckDB is the honest side.

Both are recorded in ci/known-engine-divergences.json with their evidence and
a note that recording is not endorsement. The baseline starts from this run
rather than inheriting the Python engine's allowances, which have different
causes. Keys are assertion UUIDs because that is what the gate looks up.
@dionmcm

dionmcm commented Sep 1, 2026

Copy link
Copy Markdown
Author

Found something this PR makes reachable, while running the two RVF execution engines against each other on one AU edition. Raising it here because this PR is what surfaces it, and with a fix alongside.

What happens

file-centric-snapshot-inactivated-component-module calls validate_inactivated_component_module, whose cursor is

select table_name from information_schema.tables
where table_schema = substring_index(prospective_dbname, '.', 1)
and table_name like '%\_d';

— every delta table in the schema — and the dynamic SQL it builds selects t1.id.

identifier_d has no id column. RF2 keys those rows on alternateidentifier, and the DDL in this PR declares (identifierschemeid, alternateidentifier, effectivetime, active, moduleid, referencedcomponentid). So MySQL raises

ERROR 1054 (42S22): Unknown column 't1.id' in 'field list'

which aborts the procedure. The assertion reports failureCount -1 and validates none of its components — not merely the one table it could not read.

Why this PR is where it shows up

identifier_d only exists in an RVF schema once create-tables-mysql.sql creates it, and that is one of the 12 tables added here. Before this change the cursor never saw it, so the assumption in the procedure was unreachable.

The assumption is the procedure's, not this PR's — and the RF2 file types added here genuinely had no table, which is the defect this PR fixes. I do not think the DDL should be narrowed to work around it.

Reproduced, and fixed

Against a real AU edition (853 MB, 45.3 M rows) with its previous release loaded, calling the procedure exactly as RVF does:

result
before ERROR 1054, assertion abandoned, failureCount -1
after the corpus fix executes cleanly, 0 findings, identifier_d the only table skipped

The 0 agrees exactly with what the DuckDB execution engine reports for the same assertion on the same release. That engine never hit this, because it unrolls the procedure at publish time over the 52 delta tables that have an id.

Fix raised against the assertions corpus: IHTSDO/snomed-release-validation-assertions#6 — an exists clause on information_schema.columns so the cursor yields only tables the procedure can query.

Suggestion

Land the corpus fix alongside this, or before it. On its own this PR leaves that assertion reporting -1 for any release shipping an Identifier file, which AU editions do.

How it was found

Both engines, same jar, same release, only rvf.execution.engine differing: 149 assertions joined on uuid, 147 identical failure counts, 2 divergent — this being one of them. Worth noting the check found it rather than a human reading the DDL.

dionmcm added a commit to aehrc/release-validation-framework that referenced this pull request Sep 1, 2026
The engine A/B found MySQL abandoning
file-centric-snapshot-inactivated-component-module with
"Unknown column 't1.id' in 'field list'". Cause established, reproduced, and
fixed in the assertions corpus rather than recorded as acceptable.

validate_inactivated_component_module cursors over every '%\_d' table and its
dynamic SQL selects t1.id; identifier_d is keyed on alternateidentifier and
has no id column, so the procedure aborts and the assertion validates NONE of
its components. identifier_d only exists in an RVF schema because
create-tables-mysql.sql now creates it - one of the 12 tables added by fork
PR #23 / IHTSDO IHTSDO#74 - so that change made a long-standing assumption
reachable.

Verified against the real AU edition with its previous release loaded, by
calling the procedure exactly as RVF does:

    before   ERROR 1054, assertion abandoned, failureCount -1
    after    executes cleanly, 0 findings, identifier_d the only table skipped

0 findings agrees exactly with DuckDB's 0 for the same assertion, so the two
engines now agree on 148 of 149 rather than 147.

Raised as IHTSDO/snomed-release-validation-assertions#6, with the evidence on
IHTSDO#74 and a note that landing the DDL alone leaves the assertion broken
for any release shipping an Identifier file - which AU editions do. The DDL is
not the thing to narrow: those file types genuinely had no table.

The baseline entry stays until the pinned corpus moves past that commit, and
removedAllowances now records when to delete it so the gate starts failing if
it comes back.

Also found on the way, unrelated and not chased: POST /assertions/{id}/run
throws NullPointerException because MysqlExecutionConfig.getIncludedModules()
is null and something calls .stream() on it.
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.

1 participant