sql.js icon indicating copy to clipboard operation
sql.js copied to clipboard

MATCH queries *silently* fail

Open boramalper opened this issue 5 years ago • 9 comments

I am trying to do a full-text search using FTS4, but for some reason my queries with MATCH keyword fails. I have tried both asm and wasm versions with debug and production builds but the result is the same.

The worst is that sql.js does not indicate errors by any means! You can try it on the official demo too: modify the command such that it must fail, and observe how nothing is reported.

So my questions are:

  1. Why do MATCH queries fail?
  2. Are you aware that error reporting is completely broken (on browser)?

It might be wise to write some tests to ensure this won't happen again. I'll be happy to contribute if you are interested too.

boramalper avatar Jun 03 '19 08:06 boramalper

v0.5.0 on cdnjs seem to report errors correctly, but I haven't had the opportunity to test (1).

boramalper avatar Jun 03 '19 08:06 boramalper

Yes, I would happily accept a pull request for this

lovasoa avatar Jun 03 '19 08:06 lovasoa

What about the regression on (2) with the master branch? Do you have any ideas about that?

boramalper avatar Jun 03 '19 11:06 boramalper

I've been tracking this down since the morning, and found this:

https://github.com/kripken/sql.js/blob/be04bc157f8c6d0ad34b2c62474c3a560cae575b/dist/worker.sql-wasm-debug.js#L701

By adding a console.log right before the throw statement, you can observe the errors. But I still have no idea where the thrown errors "go"...

boramalper avatar Jun 03 '19 16:06 boramalper

I wasn't aware that I broke error handling on master, and would welcome a test. This is odd. Do you have a minimal repro? What is the call stack when the error is thrown? Are you in a promise chain? If so, is it perhaps a case of not having a catch on the promise?

Ah, I just tried this on the official demo. That looks like we don't have a catch on the promise to display the error:

Uncaught (in promise) Error: near "asdfsadfsfsdf": syntax error
    at a.handleError (worker.sql-wasm.js:82)
    at a.exec (worker.sql-wasm.js:79)
    at worker.sql-wasm.js:243

Are you only seeing this on the worker examples or in the non-worker builds too?

Taytay avatar Jun 03 '19 19:06 Taytay

With regards to FTS, you can see the compilation flags we're using here: https://raw.githubusercontent.com/kripken/sql.js/master/Makefile

It looks like it's enabling FTS3 via these? -DSQLITE_ENABLE_FTS3 -DSQLITE_ENABLE_FTS3_PARENTHESIS

I know surprisingly little about SQLite, so it's possible that we're not enabling FTS4? I haven't taken the time to investigate. If you can compile your own version, you could look at adding your own compilation flags to enable FTS4 to see if that is the issue.

Taytay avatar Jun 03 '19 19:06 Taytay

Are you only seeing this on the worker examples or in the non-worker builds too?

Worker examples only. I use the official GUI example.

Ah, I just tried this on the official demo. That looks like we don't have a catch on the promise to display the error:

I can file a new issue for this if you wish. Would be great to fix this as soon as possible whilst I am investigating why MATCH queries fail.

It looks like it's enabling FTS3 via these? [...] I know surprisingly little about SQLite, so it's possible that we're not enabling FTS4?

Indeed, and no: "enabling FTS3 also makes FTS4 available. There is not a separate SQLITE_ENABLE_FTS4 compile-time option. A build of SQLite either supports both FTS3 and FTS4 or it supports neither."[1]

boramalper avatar Jun 03 '19 22:06 boramalper

Apparently the bug is observed when the database file is too large (> 2 GB, roughly). Also see #278 since it might as well be a regression introduced by #278 (I haven’t tried the traditional way of loading the whole file as a blob).

boramalper avatar Jun 12 '19 12:06 boramalper

I just ran into the lack of error-reporting in the worker builds as well. If it helps narrow things down, I've confirmed that the non-worker builds do throw errors properly (or at least dist/sql-wasm.js does).

bryangingechen avatar Aug 05 '19 15:08 bryangingechen