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

Set reading to float #776

Conversation

truongdd03
Copy link
Contributor

Description

Testing found that the space and access time are acceptable when changing the reading type from real (32-bits) to float (64-bit). This PR makes this change for the reading table and functions using these values. DB migration is also created. I have done a full migration and it works as expected on my machine.

Fixes #650

Type of change

  • Note merging this changes the node modules
  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request

Limitations

@huss
Copy link
Member

huss commented May 10, 2022

I just want to leave a record that I searched all SQL uses of real and found these:

Screen Shot 2022-05-10 at 8 47 59 AM

Given their usage they are probably fine at 32-bit so they are not being changed now.

- If you already had the compare functions then they must be dropped
before created since signature changed. I added the drop just to be
careful.
- Added comments to migration files about dropping & refreshing views
if doing manually.
Copy link
Member

@huss huss left a comment

Choose a reason for hiding this comment

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

This works fine and as intended. I did not do a full migration but tested each migration individually. I push a few small changes to clarify about order and needed operations before alters/creates. Can @truongdd03 look at them to make sure they are okay?

I note that I realized you have to refresh the reading views after the migration since there are new views. That will be dealt with separately.

I think this is ready to merge after my changes are checked.

@truongdd03
Copy link
Contributor Author

Thank you for your review and addition, @huss. New changes look good to me so I think the PR can be merged now.

@huss huss merged commit f9df6d7 into OpenEnergyDashboard:resourceGeneralization May 10, 2022
@huss huss added this to the 1.0 Release milestone May 10, 2022
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.

None yet

2 participants