Load the RF2 file types that have no table, and stop building empty schema qualifiers - #23
Open
dionmcm wants to merge 2 commits into
Open
Load the RF2 file types that have no table, and stop building empty schema qualifiers#23dionmcm wants to merge 2 commits into
dionmcm wants to merge 2 commits into
Conversation
…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.
This was referenced Aug 29, 2026
Member
Author
|
Raised upstream as IHTSDO#74. |
dionmcm
added a commit
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.
Member
Author
|
Engine A/B found that this PR makes a latent assumption in |
dionmcm
added a commit
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four MySQL-engine defects, each with a test that fails without the fix. No DuckDB involvement.
Base is
upstream-develop=IHTSDO/release-validation-framework@9b0464a3. This fork's owndevelopis 664 commits behind, so a PR against it would bury the diff.1. Four RF2 file types are never loaded
sct2_Identifier,der2_ccRefset_ExtendedAssociationandder2_csRefset_AttributeValueMapmatch no pattern inRF2FileTableMapper, sogetLegacyTableNamereturns 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_HEADERwasder2_.*Refset_.*SimpleMap, matching bothder2_sRefset_andder2_iRefset_. Split, withder2_iRefset_mapped to newisimplemaprefset_*tables. 3 more files on the same edition.create-tables-mysql.sqlgains 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_andder2_cciRefset_patterns; they matched no file uniquely and shadowed 15 that had specific patterns, so they are not here.3.
getRvfSchemaMappingproduces.langrefset_<SNAPSHOT>An unprefixed token reached the release-type suffix block with
rvfSchemastill"", giving a leading dot. That cannot parse, and it defeats theisEmpty()guard below by makingrvfSchemanon-empty.Five corpus scripts name a table this way; four are manifest-declared:
mrcm-attribute-range-refset-validate-concept-idsmrcmattributerangerefset_smrcm-domain-refset-validate-concept-idsmrcmdomainrefset_schanges-are-in-exepected-modules-validation-edition-procmoduledependencyrefset_dchanges-are-in-exepected-modules-validation-extension-procmoduledependencyrefset_dUnqualified 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, turningv_attributedescription_sinto<TEMP>.attributedescription_s.attributedescription_<SNAPSHOT>. No corpus script uses that form, so it is latent.Stored-procedure detection also missed mixed-case
CREATE PROCEDUREand allCREATE FUNCTION. Now case-insensitive, withCREATE FUNCTIONrecognised.getRvfSchemaMappingis package-private so the unit test can reach it.4. A second ENGINE clause
scripts/resource/res-table-edited-concept.sqlends... ENGINE=MEMORYand is manifest-declared. The append guard excluded only thelike/asforms, so the statement became... ENGINE=MEMORY ENGINE = MyISAM, failed, andres_concepts_editedwas 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:
Full suite against a real MySQL 8.4.6, fresh
rvf_masterbefore each run:The develop failure is
AssertionGroupImporterTest.testAssertionsImport, "Total group must be equal expected: <49> but was: <50>". It countsassertion_grouprows 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 becauseTestMySQLContainersetswithReuse(true)with a fixed port binding, so the same state can carry between runs with Docker too.