Added visit detail entity #174
Conversation
Updated master
# Conflicts: # src/main/java/org/ohdsi/circe/cohortdefinition/CohortExpressionQueryBuilder.java # src/main/resources/resources/vocabulary/sql/conceptSetQuery.sql
|
@ssuvorov-fls I'm not sure if you are attaching the 'sql rendered' sql or if you are sending me the translated form from one of the databases. Can you confirm if this is 'raw' sql or translated sql? If it is 'raw' from the sql builder, then it's incorrect because we shouldn't see things like 'interval 1 day' in the output. We should see MicrosoftSql dialect, meaning, your date-from-string should look like 'DATEFROMPARTS()' and your adding dates should be DATEADD() and difference between dates should be 'DATEDIFF()'. You should be able to look at the existing criteria templates for examples of the sql functions and how they are used. |
…entity # Conflicts: # src/test/java/org/ohdsi/circe/cohortdefinition/CohortGeneration_5_0_0_Test.java
I changed to raw version |
|
Strange formatting in your output, not sure if you formatted this or if there's another issue: Example 1: defining codeset temp table (other temp table creation has same strange formatting): Example 2: DateFromParts: Other style notes: When doing this join, we want to put the ON clause on the same line as the JOIN, but any additional AND clauses to the join can be either in-line or indented on 8000 the next line. Example: As far as the actual logic of the query, I'm going to leave that to the unit tests to confirm the proper implementation, but I think we already have a test that will check the syntax o the query, but will need tests to test the individual criteria attributes are pulling the correct rows from the test set. |
|
It seems that my ide autoformatting breaks sql code |
src/main/java/org/ohdsi/circe/check/checkers/ConceptSetCriteriaCheck.java
Outdated
Show resolved
Hide resolved
src/main/java/org/ohdsi/circe/cohortdefinition/VisitDetail.java
Outdated
Show resolved
Hide resolved
src/main/java/org/ohdsi/circe/cohortdefinition/VisitDetail.java
Outdated
Show resolved
Hide resolved
src/main/java/org/ohdsi/circe/cohortdefinition/VisitDetail.java
Outdated
Show resolved
Hide resolved
|
@chrisknoll |
| @@ -25,14 +26,17 @@ public static String getCodesetJoinExpression(Integer standardCodesetId, String | |||
| String joinExpression = ""; | |||
| ArrayList<String> codesetClauses = new ArrayList<>(); | |||
| String excludeExpression = exclude ? "<>" : "="; | |||
| String aliasPostfix = StringUtils.replaceAll(UUID.randomUUID().toString(), "-", ""); | |||
There was a problem hiding this comment.
I think you're creating this in order to support multiple joins to the #codeset table, correct? I was thinking you could do an 'in clause instead of adding joins. As in:
WHERE specialty_concept_id in (select concept_id from #codeset where codeset_id = @specialtyConceptId)
I think DBMSs can perform the necessary optimization where the in() is optimized to the same work as a join. The reason I like this form is that the criteria are all applied together in the WHERE clause and you don't have it mixed between JOINs and WHEREs.
I'm assuming that you followed the JOIN pattern from how we use the codeset_id when querying the domain tables (like for condition occurrence) but that was because, at the time, it wasn't an optional thing so putting it as part of the 'core' query made sense. But in this case, I think it would 'look' better if we just appended to the WHERE clauses (where concept_id in (select...) unless we can demonstrate a performance impact.
|
I am sorry I missed your comment about the nullCohortTest. Let me touch base with you at tomorrow's meeting and you can give me the details about the issue. |
|
I'm sorry we didn't connect on the call, so I will pull down this branch and see if i can replicate your testing error. |
|
Side note: please set your editor to 2-space indent on this repo. 2-space indent was the original format, but you'll find cases where it is 4 space indent (mostly from odysseus updates) but I want to prevent that style change from continuing. |
|
@ssuvorov-fls , I've run the nullCohortTest on my local env on this branch, and it passes. The nullCohrotTest() is just a null test when trying to call What problem were you having with the test? |
|
Circling back on this: I need to look at the final output of the query when dealing with multiple concept sets in criteria, but if I understand the code properly, when you have multiple concept sets, the join clause is being expanded to make multiple joins back to the conceptset table, for each criteria check. I think the structure of the query looks like this (with 3 concept set criteria in this example): Vs: What is nice about the second form is that the 'exclusion' form 'where we say "not in" is easy: Looking at the code, if you change the
This will return 1 row for each record that doesn't match the gender_concept_id...which is wrong, we should just return the row if it is true that none of the concepts in the concept set. Therefore, I believe the path forward is to remove the JOIN approach and use IN specifically because of the way the 'not in' should be handled. |
|
Hi @chrisknoll |
|
Thank you for your efforts on this. I was going to attempt to write some test cases to ensure that the in/not-in cases work as expected, but ran into a couple of concerns that I'd like to go over with you to figure out best way forward: CDM version 5.3 vs. 5.4
So, if any of those 5 were implemented in VisitDetail, you can remove those from the VisitDetail criteria and the UI in atlas. Test CasesI appreciate that you implemented test cases for the VisitDetail class, but, unfortunately, you altered the DDL for the cdm v5.0 when the VisitDetail was introduced in v5.3. In addition, you added test cases to the It has been a major struggle for me to find a way to cleanly make Circe support multiple CDM versions in the same version of Circe. I do have a mental idea about an approach by creating CDM-versioned CohortExpressionQueryBuilder (that embeds the correct versions of the sql builders for the given CDM version) but I haven't made any progress. I'm still thinking that maybe a version of circe is specific to one version of a CDM, but that may not be possible with multiple CDM sources in WebAPI. I did try to begin the multi-cdm-version support, which is why the test cases were split out that way, so I'd ask that you try to follow that structure. If you think that it's too much effort or complexity, let me know and I'll try to figure out a compromise. The second issue with testing was testing the 'concept-set based' criteria such has gender, provider specialty, etc. I attempted to work in those kind of tests into the WindowCriteria test cases you wrote, but noticed the above problem (that it was added to the CDM 5.0 set of tests) but also there isn't an easy way to add concept set expression Those are the remaining two issues, but they are fairly large: the v5.3 vs. 5.4 changes can be handled by not support either the 5.3 or 5.4 columns in circe, and at a later time we can add those changed columns if there is a demand for it (but only at the version 5.4 level, not supporting both v5.3 and 5.4 at the same time). The other issue on testing is something I do feel rather strongly about: we want to make sure that the codeset-based criteria works as expected, especially using the in vs. not-in options. |
|
I have pushed up a commit to re-organize the tests into cdm v5.3 groups. I would like to add a couple of tests to check the in/not-in behavior of concept-set-based criteria attributes (provider, gender, etc). I'll push up another commit for that tomorrow. If you could remove the unused criteria attributes from the object model and Atlas UI, I'll add the new tests, and that should get this done for a new release that we can push out for WebAPI 2.12. |
Updated printfriendly to handle ConceptSetSelections. Implemented tests for VisitDetail in/not in criteria.
|
@ssuvorov-fls : I've made one last push with the following changes: new class: ConceptSetSelectionAs I was writing tests for in/not in, I noticed that the Gender, PlaceOfService and ProviderSpecialty was not handling the not-in logic (it assumed it was always 'in'). Because we are going to want this capability, I've introduced a new class: With those 2 main changes, we should be all set to go with this PR. If you have any concerns, please ping me on Skype. |
|
@chrisknoll |
|
Yes, that looks correct. |
@chrisknoll, hi
Could you also check is sql query correct for visit detail criteria?
visit_detail.txt