-
Notifications
You must be signed in to change notification settings - Fork 191
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
Fix sqlite queries #1659
Fix sqlite queries #1659
Conversation
Fixes #1655 The issue was that with E+ 8.3 an index needed by certain OpenStudio queries is no longer generated by E+. Now OpenStudio creates that index if does not exist in the database.
This improves performance immensely, especially around index creation.
Blazing fast, back to 9.4 seconds :-) I'll review and merge tomorrow |
"CREATE TABLE Surfaces (SurfaceIndex INTEGER PRIMARY KEY, SurfaceName, ConstructionIndex INTEGER, ClassName TEXT, Area REAL, GrossArea REAL, Perimeter REAL, Azimuth REAL, Height REAL, Reveal REAL, Shape INTEGER, Sides INTEGER, Tilt REAL, Width REAL, HeatTransferSurf INTEGER, BaseSurfaceIndex INTEGER, ZoneIndex INTEGER, ExtBoundCond INTEGER, ExtSolar INTEGER, ExtWind INTEGER);" | ||
"CREATE TABLE SystemSizes (SystemName TEXT, Description TEXT, Value REAL, Units TEXT);" | ||
"CREATE TABLE TabularData (ReportNameIndex INTEGER, ReportForStringIndex INTEGER, TableNameIndex INTEGER, SimulationIndex INTEGER, RowNameIndex INTEGER, ColumnNameIndex INTEGER, RowId INTEGER, ColumnId INTEGER, Value TEXT, UnitsIndex INTEGER);" | ||
"CREATE TABLE EnvironmentPeriods ( EnvironmentPeriodIndex INTEGER PRIMARY KEY, SimulationIndex INTEGER, EnvironmentName TEXT, EnvironmentType INTEGER, FOREIGN KEY(SimulationIndex) REFERENCES Simulations(SimulationIndex) ON DELETE CASCADE ON UPDATE CASCADE );" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mbadams5 this does not require any change but maybe we should put the new index keys at the end of the column list? this might affect people using index based queries?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm looks like this was just wrong in that OpenStudio did not match the EnergyPlus schema.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your first comment, but the second is true but for E+ 8.3 and higher.
We need a regression type test to make sure we can read sql files from all versions of E+ 7.0 and up. I'll take a crack at this on this branch. @kbenne FYI |
Yeah there might be changes here that could fail on E+ 8.2 and earlier but i'm not entirely sure. Regression tests and/or unit tests would probably be good. |
This addresses #1655
The cause of the slow query was due to a removed index from E+ sqlite generation. This index is now automatically added each SqlFile instance if it is not already in the database.
This also fixes a couple remaining query issues and failing sql unit tests.
I also updated the SQLite library included with OpenStudio to 3.10.1. This improves performance immensely across the board for queries and index creation.
@macumber