.get() when empty, and a .getAll() Method

[tarsupin]

I'm using deno-sqlite for a webserver. So far, everything seems to be working as intended, and since this is a new project I figured I'd lend my experiences on its early development.

The largest issue comes from the fact that I'm building an SQL wrapper, which doesn't currently play nice with deno-sqlite. I need an SQL wrapper in case I switch from sqlite to an alternative at a later date. This comes with some problems, such as making the .get() methods essential to wrapping. Since I'd prefer a static entry, it also means each method has to handle the full query from db.open to db.close, and I don't have a benchmark of that cost which is concerning for large-scale use.

.get() method Technically, the biggest obstruction for a wrapper right now is the uncaught TypeError (Cannot read property '_wasm' of null) that occurs from .get() on an empty select. Requires a try/catch and unpleasant handling, especially if the wrapper is trying to send a full array of results (which mine is). I believe you've already mentioned that issues with catching will be addressed in future updates, so I'm hopeful it will extend to ._get() returning an empty object when no selects are found.

.getAll() method A .getAll() method would GREATLY improve support for SQL select wrappers. Without it, I'm afraid that deno-sqlite won't be usable for my circumstances, and I suspect for most wrappers. Resolving this should fix that.

[tarsupin]

For an example of what I'm looking for in a wrapper:

    static async select( table: string, sql: string, sqlArray: Array<any> ): Promise<any[]> {
        const db = await open(SQLHandler.DBPath + table + ".db");
        const handler = db.query(sql, sqlArray);
        const results = handler.getAll();
        handler.done();
        db.close();
        return results;
    }

[tarsupin]

I looked through the code and built a solution in rows.js (should this be in ts?) It might make sense to move the status into a shared method.

 getAll( limit = 10000 ) {
    let rows = [];
    for(let i = 0; i < limit; i++) {
        const row = this._get();
        if(row.length == 0) { break; }
        const status = this._db._wasm.step(this._db._id, this._id);
        switch (status) {
            case constants.Status.SqliteRow:
                // NO OP
                break;
            case constants.Status.SqliteDone:
                this.done();
                break;
            default:
                this.done();
                throw this._db._error(status);
                break;
        }
        rows.push(row);
    }
    return rows;
  }

[tarsupin]

While I'm at it, the crashing from no select options can be solved by changing the first four lines of .get() to this:

  _get() {
    // Get results from row
    if(!this._db) {return [];}
    const row = [];

[dyedgreen]

Thank you very much for the very detailed issue and PR! I'll be able to have a closer look at this on Friday (I'm currently finishing up exams).

From a quick read-through, I think getting the .getAll() behavior from the library should not be too hard when implementing a wrapper, by consuming the iterator directly, you could do:

const queryResult = db.query("...");

// Get all rows
const allRows = [...queryResut];

const queryResult = db.query("...");

// Get all rows with limit
const limit = 1000;
const rows = [];
for (const row of queryResult) {
  rows.push(row);
  if (rows.length >= limit)
    queryResult.done();
}

Generally the idea of returning an iterator is that you won't need to hold all results in memory at any given time and have the option to consume the one-by-one (this makes less sense with server/client based DB back-ends like MySQL, but should enable handling huge amounts of data, in conjunction with #49, using SQLite).

As I said, I only briefly looked over this, but I'll be sure to get back to you more thoroughly at the end of the week. In the meantime feel to clarify anything I might have miss-understood while glancing through.

[dyedgreen]

I have no idea why that line doesn't work since I assume it's meant to be central to the system, but as I said I've tried it multiple times in a variety of situations and it just doesn't want to cooperate in any of them. This latest example is in my select() method, which returns correctly with the results I want (with the small edit to .get()).

Yes, the line should definitely run (at least as long as you have items to return from the query, and didn't consume the iterator previously). If you are sure that you have results to return, could you provide a minimal working example to reproduce the bug, since that would definitely be a severe issue.

[tarsupin]

Sorry, you replied after I removed the comment. I just got it working, and was trying an alternative.

[dyedgreen]

And I've been thinking about rebuilding the wrapper to a non-static class to avoid the open/close repetition

Yes, that is definitely something you should do! Currently open/close requires to read the whole DB file, which is quite costly (this will be better after #49 is addressed), but still opening the database will be relatively costly, so you probably want to reuse the database handle.

[dyedgreen]

Sorry, you replied after I removed the comment. I just got it working, and was trying an alternative.

No worries 😄

[tarsupin]

Okay, I have better familiarity with this and things are operating more as I'm intending. The ._get() change also seems to be irrelevant with this. I genuinely have no idea why I was running into issues for so long when I swear I tried that exact technique a dozen times...

I might end up just iterating over everything even though my use-case doesn't particularly need it. And since this seems to be a low-level plugin, it might be the wrong place for a .getAll() method.

Anyway, I appreciate the help. I think this issue and pull request can be considered closed.