search

OHDSI / WebAPI

OHDSI WebAPI contains all OHDSI services that can be called from OHDSI applications
Apache License 2.0
128 stars 166 forks source link

Translated postgresql migration scripts to SQL Server up to v2.10 #2238

Closed dsfrederic closed 1 year ago

chrisknoll commented 1 year ago

Ok, can you confirm that these latest changes work in your environment, and i will proceed with merging the PR.

Side note: in the future (not for this PR) you may want to consider creating a branch off of your fork's master branch. I'm not sure what it will look like when we merge this commit into OHDSI/WebAPI master branch and how that will impact your own local fork's master branch. If you work off of branches, then there's no discrepancy between the commits that will be merged into OHDSI master vs. your fork's master (which will come from syncing with upstream).

dsfrederic commented 1 year ago

Well, I'm experiencing some other issues but it does not seem related.

https://github.com/microsoft/OHDSIonAzure/issues/111 https://github.com/microsoft/OHDSIonAzure/issues/89 https://github.com/OHDSI/WebAPI/issues/2210 https://forums.ohdsi.org/t/failing-webapi-atlas-implementation/18617/8

chrisknoll commented 1 year ago

Ok, one thing we found was that Hibernate is yielding a query that is OK on PG, but not OK on MSSQL: this code is defining a select/count/group by that is not doing the correct group-by clause. On one hand, we only support PG, on the other, I think it's important to yield valid SQL (PG may break later). So I think we should find a fix for this, but I don't think we're going to back-fix this back to the 2.10 release (since we're up to 2.13 now)

chrisknoll commented 1 year ago

I created an issue that will address this, but it's targeted for v2.14 (it's not exactly a hotfix, we're changing a method type, but in a way that shouldn't incur backwards-breaking change. If you are stuck on v2.10, did you want to try to make a local change on your local env to try to work around this? Note: this is a PG vs. MSSql issue which is why we decided to limit ourselves to 1 platform. Another way to work around this is host WebAPI on PG while you can still host your patient-level-data on your Azure platform...although I think the context you are working on is an all-microsoft solution (the microsoft/ohdsiOnAzure repo) but maybe the pure microsoft solution can still include a PG component?

chrisknoll commented 1 year ago

Ok, this commit was applied to fix the group-by problem. If you want to copy these changes into your own local env or cherry pick it into a custom branch for your own build purposes, I think that will get you around this issue.