Support to mysql databases #165
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mysql and co: accept connect_timeout keyword sqlite: accepts timeout keyword
cphyc
commented
Jan 14, 2022
tangos/core/creator.py
Outdated
| @@ -10,11 +10,11 @@ class Creator(Base): | |||
| __tablename__ = 'creators' | |||
|
|
|||
| id = Column(Integer, primary_key=True) | |||
| command_line = Column(String) | |||
| command_line = Column(String(128)) | |||
There was a problem hiding this comment.
I am not sure about this, in particular the command_line may be longer than 128 character. Should it be a Text instead?
Note that this change is necessary for MySQL/MariaDB databases.
There was a problem hiding this comment.
Yes I suppose it should be Text. In sqlite I think TEXT and VARCHAR are identical so this change should not cause any problems
apontzen
reviewed
Jan 14, 2022
tangos/core/creator.py
Outdated
| cwd = Column(String) | ||
| host = Column(String(128)) | ||
| username = Column(String(128)) | ||
| cwd = Column(String(128)) |
There was a problem hiding this comment.
This perhaps should also be Text
There was a problem hiding this comment.
Are you suggesting to turn host and username to Text fields as well?
apontzen
reviewed
Jan 14, 2022
tangos/core/simulation.py
Outdated
| @@ -18,7 +18,7 @@ class Simulation(Base): | |||
| # __table_args__ = {'useexisting': True} | |||
|
|
|||
| id = Column(Integer, primary_key=True) | |||
| basename = Column(String) | |||
| basename = Column(String(128)) | |||
apontzen
reviewed
Jan 14, 2022
tangos/core/simulation.py
Outdated
| @@ -111,7 +111,7 @@ class SimulationProperty(Base): | |||
| data_float = Column(Float) | |||
| data_int = Column(Integer) | |||
| data_time = Column(DateTime) | |||
| data_string = Column(String) | |||
| data_string = Column(String(128)) | |||
apontzen
reviewed
Jan 14, 2022
tangos/core/timestep.py
Outdated
| @@ -14,7 +14,7 @@ class TimeStep(Base): | |||
| __tablename__ = 'timesteps' | |||
|
|
|||
| id = Column(Integer, primary_key=True) | |||
| extension = Column(String) | |||
| extension = Column(String(128)) | |||
Otherwise this breaks the unique-ness of the entry
MySQL does not support ForeignKeys in temporary tables, so here we disable it if the table creation failed. Note that the table will be deleted when the current context is left.
The actual accuracy depends on the number of significative numbers in the database
cphyc
changed the title
…ke execution path more transparent
Refactor argmax logic into a generic function
The ambiguous order is probably what is causing intermittent failures of * test_live_calculation_link_syntax.test_missing_link * test_live_calculation_link_syntax.test_multi_calculation_link_returned_halo_is_usable with MySQL running on Github actions
…ing MySQL failures
* clarify inheritance path for Halo, BH, Tracker etc, to make clear that many relationships do not overlap * explicitly flag a few remaining relationships that overlap
Closed
Use the LONGBLOB type in MySQL to store up to 2**32 (4GiB) bytes instead of up to 2**16 (65kb)
Only TANGOS_DB_CONNECTION should be used by users.
…entation The proper argmax implementation is needed within relation_finding to make it work with MySQL (and probably other databases), but uses a large join which can hit performance (~10x degredation for complicated searches) The performance on e.g. large tree searches is now similar (though still a few percent worse than) the older, buggy code
apontzen
reviewed
Jan 30, 2022
tangos/testing/__init__.py
Outdated
| engine = create_engine(db_url) | ||
| with engine.connect() as conn: | ||
| conn.execute("COMMIT") | ||
| # Do not substitute user-supplied database names here. |
There was a problem hiding this comment.
I'm not sure I understand this comment, what's it saying?
There was a problem hiding this comment.
I think this is a leftover from a previous version, I'm going to delete it.
apontzen
reviewed
Jan 30, 2022
| @@ -103,6 +106,7 @@ def test_link_returned_halo_is_usable(): | |||
|
|
|||
| def test_multi_calculation_link_returned_halo_is_usable(): | |||
| all_links = db.get_timestep("sim/ts1").calculate_all('link(testlink)') | |||
| print("returned -->",all_links[0][0]) | |||
|
I fixed a performance issue and have left two ultra-minor comments above - as soon as they are done I think this is good to merge. Hooray! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This brings back support to non-sqlite databases.
Notably, this allows to run the database on one server with the data on another.
TODO:
TANGOS_