-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce container ordinal column in the search parameter index tables and populate it #5832
base: mm-20240318-support-for-contained-true-search-parameter
Are you sure you want to change the base?
Conversation
* failing test * fix * changelog * spotless * fix tests
…irHibernateJpaDialect (#5791) * prefixed the log message with "Unit test: " * resolved review comments
…ing (#5803) * wip * cleaning up test * adding changelog and passing spotless. --------- Co-authored-by: peartree <[email protected]>
* Reducer LOB usage in Batch2 and Search (#5748) * Reducer LOB usage in Batch2 and Search * Add changelog * Rework a number of LOB columns * Test fix * Test fix * Column fixes * Test fix * Formatting * Fixes * patching bad oracle test * Apply spotless --------- Co-authored-by: Tadgh <[email protected]> * Clean up * Revert change * One more revert * Resolve compile issue --------- Co-authored-by: Tadgh <[email protected]>
* Add support for latest version of CR and new $questionnaire operation * Fix formatting in javadoc causing warning * Update to 3.0.0 CR release * Add support for expected parameter names from updated IG's * Update Clinical Reasoning documentation * Update pom.xml * Create 5750-update-cr-operations.yaml * fix doc * Update to latest CR version * Update pom.xml * In version * Update changelog * Remove commented code
* failing test * fix * add @nullable
* Limit bulk export chunk size * Cleanup * Work on tests * Working * Add some docs * Address revuew comments * Spotless * Test fix
* Add _language param to providers and tests * add pr number to docs * fix test * remove unnecessary code --------- Co-authored-by: Lila Mikalson <[email protected]>
* update model * convert results * migration * extend test coverage in cli * fix broken test * change log * code review feedback * spotless
Stop writing to hfj_forced_id Stop reading ForcedId Cleanup client-id assignment Fix query counts in tests Drop the unused indexes and fk
…ify the capability statement (#5814) * added new customizer filter factory. have not yet deleted old code it replaces. * added new customizer filter factory. have not yet deleted old code it replaces. * replaced websocket filter. works. still some cleanup to do * replaced websocket filter. works. still some cleanup to do * cosmetic change * add coverage and fix bugs it found * spotless * move capability statement classes * add changelog and rename new classes * review feedback
* step one of unifying the bulk exports * adding changelog * review points * spotless --------- Co-authored-by: leif stawnyczy <[email protected]>
* refactoring * test refactor only * spotless * bumping version --------- Co-authored-by: leif stawnyczy <[email protected]>
* licenses * remove mdm placeholder interceptor * remove mdm placeholder interceptor * fix regression * Remove RTE Placeholder Interceptor. Add tests to assert proper boot sequence. * consolidate LogbackCaptureTestExtension (removed the cdr class) * consolidate LogbackCaptureTestExtension (removed the cdr class)
* adding a catch for failing reduction step * spotless * added changelog --------- Co-authored-by: leif stawnyczy <[email protected]>
…late the value when the indexes are built. Refactor to decouple entities from settings and from hashing logic.
Formatting check succeeded! |
…itionSubscriptionEnabled if it's true. Make that setting is true by default (#5810) * First commit with TODOs and logging. * Try to add cross partition config at startup to subscription module. * Barely working solution with JpaStorageSettings injected into the Subscription module with the correct config for cross partition enabled. * Implement agreed upon solution where StorageSettings used in the subscription module uses the JpaStorageSettings cross partition enabled setting. Fix all compile errors. TODOs for tests to add and known test failures. * Fix test errors caused by bad log code. Ensure all modules use StorageSettings for canonicalizer. * Cleanup. * Reintroduce old SubscriptionCanonicalizer constructor, but add a StorageSettings and deprecate it. Cleanup logs and TODOs. * Deprecate FHIR_PATCH. More cleanup. * Deprecate FHIR_PATCH correctly. * Small fix. * Set myCrossPartitionSubscriptionEnabled to true by default. * Fix test failures. * Fix another test. * Code review feedback. * Resolve static analysis warnings.
…ce-container-ord-seach-parameter-and-populate
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## mm-20240318-support-for-contained-true-search-parameter #5832 +/- ##
==========================================================================================
Coverage ? 83.44%
Complexity ? 26920
==========================================================================================
Files ? 1688
Lines ? 104025
Branches ? 13195
==========================================================================================
Hits ? 86805
Misses ? 11578
Partials ? 5642 ☔ View full report in Codecov by Sentry. |
private transient StorageSettings myStorageSettings; | ||
@GenericField | ||
@Column(name = "CONTAINED_ORD", nullable = true) | ||
private Integer myContainedOrd; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: should this be Short instead? The migration used tinyint which is just 2 bytes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I had updated this to Short
in one of the sub-sequent commits, you're probably seeing an older version: https://github.com/hapifhir/hapi-fhir/pull/5832/files#diff-78a0ab6d4a89738d68dc1c231adcd7a75c64a81ee918cdcde761dde8b4f48637R69
public void assignIdsToContainedResources() { | ||
|
||
if (!getContainedResources().isEmpty()) { | ||
public static class ContainedResources extends EmbeddedResources { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: Do we need a new EmbeddedResources intermediate class? Is there a reason we can't apply ordinals to the other cases?
problem: I can't find a test for the ordinal assignment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Embedded resources can also mean resource entries in a bundle. Basically any resource that's embedded into another resource. I guess there may even be other cases. I kept is separate.
I added this test here for now: https://github.com/hapifhir/hapi-fhir/pull/5832/files#diff-d9ea0db9e7f4eb2987dbfb2fa7376d67cac04733f6fca71caeaa363bf207b12aR119. I plan to add some more after I implement the query/search part of the functionality.
@@ -121,7 +121,7 @@ public void testMigrateFrom340() throws IOException, SQLException { | |||
assertEquals("identifier", values.get(0).get("SP_NAME")); | |||
assertEquals("12345678", values.get(0).get("SP_VALUE")); | |||
assertTrue(values.get(0).keySet().contains("HASH_IDENTITY")); | |||
assertEquals(7001889285610424179L, values.get(0).get("HASH_IDENTITY")); | |||
assertEquals(792023395537367244L, values.get(0).get("HASH_IDENTITY")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
serious problem: we must never change the hashes of existing data. Why did this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the bit in the hash (boolean) to distinguish contained indexes. I guess I can always leave as is if the property to index them is false. I did think about it, the impact would be less it just requires more branching in the code.
import java.util.List; | ||
import java.util.concurrent.atomic.AtomicInteger; | ||
|
||
public class HapiFhirMigrationTasksV7 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: Hapi style is to keep adding them to he main class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I guess it's ok if the class becomes gigantic then. I will move it there.
|
||
// Make sure our hashing function gives consistent results | ||
assertEquals(6598082761639188617L, token.getHashNormalizedPrefix().longValue()); | ||
assertEquals(-1970227166134682431L, token.getHashExact().longValue()); | ||
assertEquals(-3910281295602822115L, token.getHashNormalizedPrefix().longValue()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
catastrophic: these hashes are wrong. They must never change.
@@ -2827,7 +2827,7 @@ public void testTransaction_MultipleConditionalUpdates() { | |||
myCaptureQueriesListener.logSelectQueriesForCurrentThread(); | |||
assertEquals(2, myCaptureQueriesListener.countSelectQueriesForCurrentThread()); | |||
assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false), containsString("rispt1_0.PARTITION_ID in ('1')")); | |||
assertThat(myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false), containsString("rispt1_0.HASH_SYS_AND_VALUE in ('7432183691485874662','-3772330830566471409','-4132452001562191669')")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
problem: hash
@@ -69,7 +69,7 @@ public void testRawSql_Search() { | |||
String query = list.get(0).getSql(true, false); | |||
ourLog.info("Query: {}", query); | |||
|
|||
assertThat(query, containsString("HASH_SYS_AND_VALUE = '3788488238034018567'")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
problem: hash
task.addCalculator("HASH_SYS", t -> ResourceIndexedSearchParamToken.calculateHashSystem(new PartitionSettings(), RequestPartitionId.defaultPartition(), t.getResourceType(), t.getParamName(), t.getString("SP_SYSTEM"))); | ||
task.addCalculator("HASH_SYS_AND_VALUE", t -> ResourceIndexedSearchParamToken.calculateHashSystemAndValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), t.getResourceType(), t.getParamName(), t.getString("SP_SYSTEM"), t.getString("SP_VALUE"))); | ||
task.addCalculator("HASH_VALUE", t -> ResourceIndexedSearchParamToken.calculateHashValue(new PartitionSettings(), RequestPartitionId.defaultPartition(), t.getResourceType(), t.getParamName(), t.getString("SP_VALUE"))); | ||
task.addCalculator("HASH_IDENTITY", t -> hasher.hash(RequestPartitionId.defaultPartition(), t.getResourceType(), t.getString("SP_NAME"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: I much prefer the explict names like calculateHashSystemAndValue than an anonymous list of strings whose order is important, but unspecified.
assertEquals(2686400398917843456L, map.get("HASH_SYS")); | ||
assertEquals(-3943098850992523411L, map.get("HASH_SYS_AND_VALUE")); | ||
assertEquals(845040519142030272L, map.get("HASH_VALUE")); | ||
assertEquals(792023395537367244L, map.get("HASH_IDENTITY")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
problem: change hash
@@ -50,7 +50,7 @@ public void testOrdinalSearchesUseIntegerParameters() { | |||
|
|||
assertEquals(4, StringUtils.countMatches(sql, "?")); | |||
assertEquals(4, generatedSql.getBindVariables().size()); | |||
assertEquals(123682819940570799L, generatedSql.getBindVariables().get(0)); | |||
assertEquals(45648902074593574L, generatedSql.getBindVariables().get(0)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
problem: change hash
|
||
AtomicInteger i = new AtomicInteger(); | ||
indexTableList.forEach(idxTable -> { | ||
Builder.BuilderWithTableName tableBuilder = myVersion.onTable(idxTable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: this is a bit fancy for migrations. One of the challenges of migrations is ensuring that old migrations don't change as we write new code. We should think of old migrations as a historical document, not living code. But fancy code like this tempts refactoring. We prefer simple and explicit here. Even more so given the production problems caused by bad migrations.
#5285
The work on this feature has been paused as of mid-April. It can be resumed in future at which point this PR can be updated or cancelled and (some) the changes can go into another PR.