Skip to content

Conversation

dsfrederic
Copy link
Contributor

No description provided.

@chrisknoll
Copy link
Collaborator

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
Copy link
Contributor Author

dsfrederic commented Mar 30, 2023

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

microsoft/OHDSIonAzure#111
microsoft/OHDSIonAzure#89
#2210
https://forums.ohdsi.org/t/failing-webapi-atlas-implementation/18617/8

@chrisknoll
Copy link
Collaborator

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
Copy link
Collaborator

chrisknoll commented Mar 31, 2023

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
Copy link
Collaborator

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.

@chrisknoll chrisknoll merged commit 8403145 into OHDSI:master Apr 4, 2023
pieterlukasse pushed a commit to pieterlukasse/WebAPI that referenced this pull request Sep 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants