Skip to content

Conversation

@jeremyestein
Copy link
Collaborator

@jeremyestein jeremyestein commented Mar 7, 2025

SQL Server did not like the following types we were manually specifying:

  • bytea
  • timestamp with time zone
  • double precision array

The general tactic has been to remove these manual types from @Column annotations, and create three custom SQL dialects: for postgres, SQL server, and H2 which specifies them instead on a per-backend basis.

SQL Server does not in fact support SQL arrays, and I haven't attempted to reimplement them in another way, so waveform data will not work in SQL Server without further effort.

The current status is that the DDL runs nicely on my machine, but since we don't have any system tests (except for the waveform-generator which doesn't work in SQL Server anyway), I will have to test this on the GAE to see if real data gets put in.

@jeremyestein jeremyestein requested a review from stefpiatek March 7, 2025 19:57
Copy link
Collaborator

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

Well that's fun, once we've got an example with this working let us know - would be keen to give to information services and have them be able to toy around with it

InformDB:
emap:
branch: develop
hl7-vitals:
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this still be here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hl7-vitals? Not sure, I don't recognise that name. It might be trying to test having a different checkout directory to the repo name, but changing any of those values doesn't break any additional tests so who knows.

Copy link
Collaborator

@stefpiatek stefpiatek May 8, 2025

Choose a reason for hiding this comment

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

this is an old name and should be removed

```
- Timestamps (date and times) are timezone aware (and automated testing enforces this) and named in the form `<something>Datetime`
- Dates should only have date information and be named in the form `<something>Date`
- `timestamp with time zone` is Postgres-specific so should not be specified manually. Hibernate does the right thing in this case.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🎉

Copy link
Collaborator

@stefpiatek stefpiatek left a comment

Choose a reason for hiding this comment

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

This is looking nice, Steve is really grateful to see this working 🙏🏻

Comment on lines +17 to +32
fakeuds-mssql:
image: mcr.microsoft.com/mssql/server:2019-CU32-ubuntu-20.04
env_file:
- ../../config/fakeuds-mssql-config-envs
environment:
ACCEPT_EULA: "Y"
MSSQL_PID: "Express"
TZ: "Europe/London"
volumes:
- mssql-data-fakeuds:/var/opt/mssql
restart: unless-stopped
ports:
- "${FAKEUDS_MSSQL_PORT}:1433"
profiles:
- fakeuds-mssql

Copy link
Collaborator

Choose a reason for hiding this comment

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

semantics but I'd argue that this isn't a fake UDS, instead its a mssql-star or just mssql-db

# dialect to use for core informdb tables
UDS_HIBERNATE_DIALECT: uk.ac.ucl.rits.inform.informdb.PostgreSQLEmapDialect
# dialect for modules that don't have access to emap-star (which contains the above code)
UDS_HIBERNATE_DIALECT_2: org.hibernate.dialect.PostgreSQL95Dialect
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would this be a more meaningful name?

Suggested change
UDS_HIBERNATE_DIALECT_2: org.hibernate.dialect.PostgreSQL95Dialect
UDS_HIBERNATE_DIALECT_FALLBACK: org.hibernate.dialect.PostgreSQL95Dialect

* ever needed to use more than one type of SQL array. Hopefully we will have
* found a better way of storing waveform data before then!
*/
public PostgreSQLEmapDialect() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

fun!

@jeremyestein
Copy link
Collaborator Author

(reason for force push: removing changes to deployment documentation that are now in their own PR #124 )

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