TIMX 530 - rebuild TIMDEXDatasetMetadata with static database file approach#157
Conversation
Why these changes are being introduced: The current overarching work is to support the creation and reading of a static metadata database file and append deltas. To get there, very little of the original TIMDEXDatasetMetadata class is needed or wanted. This commit begins the process of rebuilding TIMDEXDatasetMetadata, oriented around managing a static metadata database file, and providing a readonly projection over that and append delta paqruet files. How this addresses that need: TIMDEXDatasetMetadata is almost completely rebuilt, with the first functionality being the creation of the static metadata file by scanning the ETL records. Then, the ability to remotely attach in readonly mode to this metadata database file for reading. Note: these changes are breaking. TIMDEXDataset cannot provide "current" records and many unit tests are broken. This will be addressed in future commits as we build this class back up with new functionality. Side effects of this change: * TIMDEXDataset cannot provide current records * Unit tests are either temporarily skipped or failing Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-530
Why these changes are being introduced: While the TIMDEXDatasetMetadata class is rebuilt, TIMDEXDataset itself can no longer provide "current" records from the dataaset as it has no metadata to work with. This is temporary until TIMDEXDatasetMetadata is rebuilt, and TIMDEXDataset gets new functionality based on *that* new metadata. How this addresses that need: * Any reference to "current records" is removed Side effects of this change: * TIMDEXDataset cannot provide current records Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-530
Why these changes are being introduced: This is a small change now, that will lead to a larger change later. The TIMDEX dataset is getting more structure, and this means we will want to initialize a TIMDEXDataset instance with the root of the dataset, but then internally there will be more opinionation about where files should be read and written to. How this addresses that need: A new property 'data_records_root' is added to TIMDEXDataset that mirrors similar properties in TIMDEXDatasetMetadata. This informs any operations that need to read or write ETL records where precisely they are in the dataset. At this time only .write() utilizes it, but in a future ticket the load method will be heavily reworked (if not outright removed) and this property will be fully integrated. This is needed now to continue updates to TIMDEXMetadataDataset for TIMX-530. Side effects of this change: * Initialization of TIMDEXDataset should provide the true dataset root, not point to /data/records. The pipeline lambda currently does this, but will be updated in TIMX-531. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-530 * https://mitlibraries.atlassian.net/browse/TIMX-531
Why these changes are being introduced: With the big changes to TIMDEXMetadataDataset comes the need to virtually rewrite the test suite for that class. The changes too TIMDEXMetadataDataset are also influencing tests for TIMDEXDataset, both how its loaded and tested for 'current' record reading. How this addresses that need: This begins with some basic tests around the loading, creating, and attaching of a static database file for TIMDEXMetadataDataset. Future tests will more fully exercise the final views and tables created. This commit also *temporarily* skips a bunch of tests for TIMDEXDataset that will not pass until the ability to limit to 'current' records is reinsated with the updated TIMDEXMetadataDataset. Side effects of this change: * Test suite passes, but multiple tests are temporarily skipped. Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/TIMX-530
27aeb0b to
7f7800d
Compare
Pull Request Test Coverage Report for Build 16780878765Details
💛 - Coveralls |
jonavellecuerdo
left a comment
There was a problem hiding this comment.
Looks great! Just have a couple questions. My main change request / question was on whether we should keep referring to the DuckDB file as a "static" DuckDB file (see comment for more details).
I also wondered:
- Whether the performance on my local machine was similar to yours:
-
Whether we should add a DEBUG log also describing the time elapsed after the "Attaching to static database file" log message
-
Where the 'database_name_varchar' comes from:
Yep! Looks about right. The static metadata file recreation takes anywhere from 30-45 seconds. And that will grow over time, but we don't need to perform very often.
If you're comfortable with it, I'll add during TIMX-526 which is focused heavily on setting up the DuckDB context! In fact, might be in there already...
That's just a DuckDB convention for the DuckDB query `"show databases;". For a lot of DuckDB (and other DBs for that matter) their "meta" commands like this often have kind of awkward column names because it's just communicating data. Actually, I'm realizing that's kind of two lines! It's "database_name" as the column name, and "varchar" as the column type. |
Purpose and background context
This PR begins effectively a rewrite of
TIMDEXMetadataDataset, and the process of incrementally updatingTIMDEXDatasetto align with those changes. Major changes:TIMDEXMetadataDataset, building up functionality over the next few PRsTIMDEXDatasettemporarily loses the ability to filter to only "current" recordsTIMDEXDatasetis loaded to expect the root of the dataset, then internally know where the ETL parquet records areThis is probably the trickiest PR for epic TIMX-515. To begin building up both
TIMDEXMetadataDatasetandTIMDEXDatasetto more fully utilize dataset metadata, it requires temporarily stripping back functionality and unit testing.For this review, I would ask reviewers to focus most heavily on
TIMDEXMetadataDatasetcreating and attaching to a static metadata file as the foundation for dataset metadata. Future PRs will layer on append deltas, create views, adjust how we read data, and the associated tests.Planned, ordered next steps:
TIMDEXDataset.load()and dataset "location", TIMX-533How can a reviewer manually see the effects of these changes?
Despite things being in a liminal state, manual testing is possible.
1- Set AWS Dev
TimdexManagerscredentials2- Set env vars:
3- Start Ipython shell with
pipenv run ipythonand do some setup:4- First, note small update to
TIMDEXDatasetwhere it knows both the dataset root location and the location of the ETL parquet files:5- Recreate the dataset metadata:
6- Perform very simple query demonstrating the metadata database file was created and is successfully attached:
7- If interested, reload the
TIMDEXDatasetMetadatainstance and see the payoff of all this work in nearly instant dataset metadata available!Ticket TIMX-526 will create all the helpful tables and views once the append deltas exist as well.
Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO: still only merging to feature branch
epic-TIMX-515What are the relevant tickets?