Skip to content

Implement SQLite#68

Open
drekdrek wants to merge 18 commits intomainfrom
sqlite
Open

Implement SQLite#68
drekdrek wants to merge 18 commits intomainfrom
sqlite

Conversation

@drekdrek
Copy link
Copy Markdown
Owner

@drekdrek drekdrek commented May 1, 2025

closes #67

@drekdrek
Copy link
Copy Markdown
Owner Author

drekdrek commented May 1, 2025

this is going to require something similar to when we moved the data to the PluginStorage folder. a whole migration process that happens when someone loads the plugin with this update

@drekdrek
Copy link
Copy Markdown
Owner Author

drekdrek commented May 1, 2025

Recap also will need to be moved over

edit: this will require a large re-write of recap

@drekdrek
Copy link
Copy Markdown
Owner Author

drekdrek commented May 2, 2025

all thats left is migrating the old data into sqlite

@drekdrek
Copy link
Copy Markdown
Owner Author

drekdrek commented May 2, 2025

currently in my tests with ~17,000 files it takes approx. 2 minutes and 30 seconds to migrate all files into the SQL db, i believe that the slowest part of this is definitely the medals, because its usually about 7 medals per map. I'll be optimizing this more.

@drekdrek drekdrek marked this pull request as ready for review June 8, 2025 12:02
Copy link
Copy Markdown
Contributor

@unnecessarilycomplex unnecessarilycomplex left a comment

Choose a reason for hiding this comment

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

this looks almost complete I think. I haven't tested the branch but let me know if it would be useful

Comment on lines +65 to +83
string grinds_query_string = """
SELECT * FROM grinds WHERE map_id = ?;
""";

auto grinds_query = db.Prepare(grinds_query_string);
grinds_query.Bind(1, mapUid);
grinds_query.Execute();

grinds_query.NextRow(); // im not sure why i have to do this twice, but its blank otherwise
grinds_query.NextRow();

finishes = uint64(grinds_query.GetColumnInt64("finishes"));
resets = uint64(grinds_query.GetColumnInt64("resets"));
time = uint64(grinds_query.GetColumnInt64("time"));
respawns = uint64(grinds_query.GetColumnInt64("respawns"));


debug_print("Loaded finishes " + finishes + " resets " + resets + " time " + time +
" respawns " + respawns + " with map_id " + mapUid);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For readability this block could be put this in a separate load_grinds() method. etc for load_medals(), save_grinds()...

@@ -0,0 +1,182 @@
class SQLite : AbstractData {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The class name SQLite conflicts with the namespace name SQLite

@this.db = db;
initialize();

try {migrate();} catch {}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't all these issues be handled by migrateToSQLite() in main ?

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

this migrate() function is in case more columns need to be added later, which then a db migration would be necessary

Comment on lines 16 to +17
migrateOldData();
startnew(CoroutineFunc(migrateToSQLite));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this should be

migrateOldData();
migrateToSQLite();
data.start()

p.s. Also I don't understand either why new coroutines are used at startnew(CoroutineFuncUserdata(insert_medals),strings); and startnew(CoroutineFunc(mergeData)); and startnew(CoroutineFunc(TurboSTM::LoadSuperTimes));. Can't this cause concurrency issues ?

return;
}
@this.db = db;
initialize();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this would be better named check_tables_exist() (or init_tables_if_not_exist()) as 99.9% of the time this will not initialize tables



auto old_path = IO::FromStorageFolder("data");
if (IO::FolderExists(old_path)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This also needs to check if the database tables are empty.

}

void insert_medals(ref@ medals_ref) {
array<string> strings = cast<array<string>>(medals_ref);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

whitespace

int BATCH_AMOUNT = 50;
uint batches = uint(Math::Ceil(old.Length / float(BATCH_AMOUNT)));
for (uint i = 0, idx = 0; i < batches; i++) {
uint amt = Math::Min(BATCH_AMOUNT, Math::Max(0, old.Length - (i * BATCH_AMOUNT)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Math::Max confuses me, is it necessary ? (as the last value of i is batches - 1)

strings.InsertAt(1, medals_string);


startnew(CoroutineFuncUserdata(insert_medals),strings);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This creates one coroutine and one insert per map. This kills any performance gains from batching on grinds

I think the structure would be clearer as

for each batch
    batch_map_ids = (parse filenames)
    batch_jsons = array of Json::Values for each map
    insert_into_grinds(batch_maps, batch_jsons)
    insert_into_medals(batch_maps, batch_jsons)

and then

void insert_into_grinds(batch_maps, batch_jsons) {
    statement = "INSERT INTO ..."
    for each map
        statement += "..."
    for each map
        parse json
        statement.Bind
    execute statement
}

and same for medals


SQLite::Database@ db = SQLite::Database(IO::FromStorageFolder("data.db"));
// Cloud @cloudData = Cloud();
Files @localData = Files();
Copy link
Copy Markdown
Contributor

@unnecessarilycomplex unnecessarilycomplex Jul 10, 2025

Choose a reason for hiding this comment

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

as far as I understand the Files class isn't used anymore so the source file may be removed from the codebase (?)

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.

Unify map data

2 participants