-
Notifications
You must be signed in to change notification settings - Fork 94
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
SQLite utility methods #366
Conversation
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.
Hey. I had a look here, and I can't by eye gauge if this works as intended. I would need to test these new changes to make sure everything works as intended.
Because parquet_to_sqlite_converter
relies on these sqlite functions, which are also used by the sqlite_data_converter
, we could end up in a situation where we change something based on usage of the sqlite_data_converter
that breaks the parquet_to_sqlite_converter
. So a unit test might be a good idea here.
The functions that are used in |
Alright, have added a unit test now, @RasmusOrsoe. The existing unit test converts a test I3-file using both |
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.
Hey! I think this is great.
Could we add a second test that checks that the tables are indexed correctly? I have a conceptual snippet here:
def test_search_plan(sqlite_database, parquet_converted_database, pulsemap, event_no = 1):
with sqlite3.connect(sqlite_database) as con:
query = f'EXPLAIN QUERY PLAN SELECT * FROM {pulsemap} WHERE event_no={event_no}'
sqlite_plan = pd.read_sql(query,con)
with sqlite3.connect(parquet_converted_database) as con:
query = f'EXPLAIN QUERY PLAN SELECT * FROM {pulsemap} WHERE event_no={event_no}'
parquet_plan = pd.read_sql(query,con)
assert 'USING INDEX event_no' in sqlite_plan['detail']
assert 'USING INDEX event_no' in parquet_plan['detail']
assert sqlite_plan['detail'] == parquet_plan['detail']
return
Testing one event_no
is enough pr. pulsemap
…imised table creation
There we go, @RasmusOrsoe: Have now added a unit test like the one suggested, and... it actually caught a (seemingly ancient) bug! 🎉 One place in the |
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.
Awesome!!
I will sleep better after this :-)
…hods SQLite utility methods
create_table
andattach_index
-like functionssave_to_sql
in src/graphnet/data/utilities/parquet_to_sqlite.py that looked backwards to me. Please check whether I'm mistaken here, @RasmusOrsoe!Closes #358
cc @Peterandresen12