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

Add sqlite backend #751

Merged
merged 3 commits into from
Feb 6, 2019
Merged

Add sqlite backend #751

merged 3 commits into from
Feb 6, 2019

Conversation

BrendanBall
Copy link
Contributor

@BrendanBall BrendanBall commented Dec 9, 2018

This change is Reviewable

@BrendanBall BrendanBall requested a review from dennwc as a code owner December 9, 2018 18:33
Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

The code looks good, I think we can merge it right away. I only want to run tests locally to make sure everything is in order.

Also, can you please update README.md, ./docs/Configuration.md and any other places that list supported backends (just search for postgres in the code)?

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dennwc dennwc added this to the v0.7.6 milestone Dec 9, 2018
@dennwc dennwc added the backends label Dec 9, 2018
@BrendanBall
Copy link
Contributor Author

Thanks for the awesome response time :D . I've just pushed some docs. Let me know if you'd like to add anything else.

Copy link
Member

@dennwc dennwc left a comment

Choose a reason for hiding this comment

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

Thanks for adding the documentation! :) Also make sure to add yourself to AUTHORS and CONTRIBUTORS.

All regular tests pass (:tada:), but I've hit an error when running the integration test suite (RUN_INTEGRATION=true go test ./...). Haven't checked yet what the reason is, though.

@BrendanBall
Copy link
Contributor Author

Cool, I'll check on that tomorrow

@BrendanBall
Copy link
Contributor Author

BrendanBall commented Dec 11, 2018

So here are the failing tests:

➜  cayley git:(master) ✗ RUN_INTEGRATION=true go test ./graph/sql/sqlite -timeout 30m
--- FAIL: TestSqlite (0.00s)
    --- FAIL: TestSqlite/qs (1020.98s)
        --- FAIL: TestSqlite/qs/integration (1020.88s)
            integration.go:490: loaded data in 18.9748405s
            --- FAIL: TestSqlite/qs/integration/three_huge_sets_with_small_intersection (300.00s)
                integration.go:523: Error: interrupted
                integration.go:533: 5m0.000410593s three huge sets with small intersection
                integration.go:536: Unexpected number of results, got:0 expect:2 on three huge sets with small intersection.
            --- FAIL: TestSqlite/qs/integration/Keanu_with_other_in_The_Net (300.00s)
                integration.go:533: 5m0.000397876s Keanu with other in The Net
                integration.go:536: Unexpected number of results, got:0 expect:2 on Keanu with other in The Net.
            --- FAIL: TestSqlite/qs/integration/Keanu_and_Bullock_with_other (300.00s)
                integration.go:533: 5m0.000344452s Keanu and Bullock with other
                integration.go:536: Unexpected number of results, got:6 expect:166 on Keanu and Bullock with other.
FAIL
FAIL	github.com/cayleygraph/cayley/graph/sql/sqlite	1021.010s

No idea why those 3 tests are failing. Each of those take 5 min to run so it's pretty difficult to debug. Might take a while to figure out.

EDIT: So I've found that the query timeout for the tests is set to 5 min, which is why all 3 these tests fail at 5 min. I'm not sure how long the other backends take to run these tests, but either sqlite is insanely slow for this case or these test queries are pretty crazy.

@dennwc
Copy link
Member

dennwc commented Dec 11, 2018

@BrendanBall That might explain why they fail for the same SQL implementation that works for PG and MySQL.

I was confused by Unexpected number of results, got:0 expect:2 without any errors, but that's probably a problem with context error propagation in iterators.

If setting a higher timeout helps, I will consider this solved.

Sure, we might need some optimizations for SQLite to be faster for those kinds of queries, but it's worth merging in the current state as well.

@BrendanBall
Copy link
Contributor Author

the test TestSqlite/qs/integration/three_huge_sets_with_small_intersection didn't even complete within an hour :/ . So much for quickly adding a sqlite backend :P

@dennwc
Copy link
Member

dennwc commented Jan 7, 2019

OK, I would say that the fact that backend is slow on integration test shouldn't block this PR. We can make some optimizations later, if necessary.

I will check the PR again and merge it if there are no objections.

@dennwc dennwc merged commit 7081602 into cayleygraph:master Feb 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants