Skip to content
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

MATCH queries *silently* fail #279

Open
boramalper opened this issue Jun 3, 2019 · 9 comments
Open

MATCH queries *silently* fail #279

boramalper opened this issue Jun 3, 2019 · 9 comments

Comments

@boramalper
Copy link

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
Copy link
Author

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

@lovasoa
Copy link
Member

lovasoa commented Jun 3, 2019

Yes, I would happily accept a pull request for this

@boramalper
Copy link
Author

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

@boramalper
Copy link
Author

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"...

@Taytay
Copy link
Contributor

Taytay commented Jun 3, 2019

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
Copy link
Contributor

Taytay commented Jun 3, 2019

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.

@boramalper
Copy link
Author

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
Copy link
Author

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).

@bryangingechen
Copy link
Contributor

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).

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

No branches or pull requests

4 participants