Skip to content
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

Use external concept counts #1136

Open
wants to merge 10 commits into
base: develop
Choose a base branch
from
Open

Use external concept counts #1136

wants to merge 10 commits into from

Conversation

ablack3
Copy link
Collaborator

@ablack3 ablack3 commented Sep 19, 2024

Moving this PR to the OHDSI organization so we can run all the tests in github actions.

@cebarboza cebarboza marked this pull request as ready for review September 19, 2024 13:00
Copy link
Collaborator

@azimov azimov left a comment

Choose a reason for hiding this comment

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

Only minor changes suggested - add when ready.

A possible change if you want would be to make the CD concept counts table here identical to the fields needed in the achilles table - before I go and check @ablack3 @cebarboza, are they substantially different?

conceptCountsTable <- "#concept_counts"
}
} else {
if (conceptCountsTable == "#concept_counts") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably be substr(conceptCountsTable, 1, 1) == "#" as otherwise #my_temp_table will render as work_database_schema.#my_temp_table which will crash on some systems

snakeCaseToCamelCase = TRUE,
tempEmulationSchema = getOption("sqlRenderTempEmulationSchena")
)
if (!identical(vocabVersion, vocabVersionExternalConceptCountsTable[1,1])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

great check!

if (dbms == "sqlite") {
# Creating externalConceptCounts
sql_lite_path <- file.path(test_path(), databaseFile)
connectionDetails <- createConnectionDetails(dbms= "sqlite", server = sql_lite_path)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We may want to build tests out for real databases but this can likely wait - I think adding a pre-computed counts table should speed up the other unit tests

@ablack3
Copy link
Collaborator Author

ablack3 commented Sep 20, 2024

A possible change if you want would be to make the CD concept counts table here identical to the fields needed in the achilles table - before I go and check @ablack3 @cebarboza, are they substantially different?

The achilles table we would like to use has "concept_id", "record_count", "descendant_record_count", and we would like to add "person_count" and "descendant_person_count" so we could use those column names.

https://github.com/OHDSI/Achilles/blob/247b86da872ceeb80b70d35901faa6795d3ab0d8/inst/sql/sql_server/analyses/create_result_concept_table.sql#L31

@ablack3
Copy link
Collaborator Author

ablack3 commented Sep 20, 2024

We are adding two parameters that depend on each other: conceptCountsTable (temp or not temp) and useExternalConceptCountsTable (T or F).

Let's list out the expected behavior:

T F
temp error? created as temp table. replace if it already exists.
permanent look for the table in the cohort schema. if it does not exist create it. ignore conceptCountsTable parameter. create temp table. replace if it already exists.

I'm wondering if we could do this with just one parameter, the cohortCountsTable. if it exists we use it and if not we recreate it.

@azimov
Copy link
Collaborator

azimov commented Sep 30, 2024

@cebarboza @ablack3 I'm going to release 3.3.0 today for the HADES wide lock file so this will make its way in to a 3.4.0 release whenever it becomes ready

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.

3 participants