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

SQLite integration #24194

Merged
merged 36 commits into from
Jul 13, 2021
Merged

SQLite integration #24194

merged 36 commits into from
Jul 13, 2021

Conversation

g-arslan
Copy link
Contributor

@g-arslan g-arslan commented May 17, 2021

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Add SQLite table engine, table function, database engine.

@robot-clickhouse robot-clickhouse added doc-alert pr-feature Pull request with new product feature labels May 17, 2021
@robot-ch-test-poll2 robot-ch-test-poll2 added the submodule changed At least one submodule changed in this PR. label May 17, 2021
@kssenii kssenii self-assigned this May 18, 2021
@kssenii
Copy link
Member

kssenii commented May 18, 2021

Please, remove submodules update, which was added by accident -- looks like all of them were added in the last commit, then just revert it. And sqlite library should be added as submodule - you can delete contrib/sqlite and then add it again by executing in contrib dir git submodule add ...- that's all, then you'll have to commit .gitmodules file and the submodule.

Also, please make integration tests in ClickHouse/tests/integration/test_sqlite_storage and test_sqlite_database_engine (see example in tests/integration/test_postgresql_database_engine and tests/integration/test_postgresql_storage).

Also it will be very easy to add sqlite table function - can be implemented in a few minutes since sqlite storage and method fetchSQLiteTableStructure are already implemented, it also should be added to integration tests.

@alexey-milovidov
Copy link
Member

alexey-milovidov commented May 18, 2021

We can add functional tests instead of integration tests - it's easy, because sqlite database is a single file.

@alexey-milovidov
Copy link
Member

Tables should be also writable.

src/CMakeLists.txt Outdated Show resolved Hide resolved
src/CMakeLists.txt Outdated Show resolved Hide resolved
src/DataStreams/SQLiteBlockInputStream.cpp Outdated Show resolved Hide resolved
src/Databases/SQLite/DatabaseSQLite.cpp Outdated Show resolved Hide resolved
src/Databases/SQLite/DatabaseSQLite.cpp Outdated Show resolved Hide resolved
src/Databases/SQLite/DatabaseSQLite.cpp Outdated Show resolved Hide resolved
src/Databases/SQLite/fetchSQLiteTableStructure.cpp Outdated Show resolved Hide resolved
src/Storages/StorageSQLite.cpp Outdated Show resolved Hide resolved
src/Storages/StorageSQLite.cpp Outdated Show resolved Hide resolved
@kssenii
Copy link
Member

kssenii commented Jun 2, 2021

ubsan caught something :)

01889_sqlite_read_write: [ FAIL ] 13.61 sec. - return code 210
2021-06-02 14:14:57 [58e2ccd64857] 2021.06.02 14:14:46.237369 [ 107873 ] BaseDaemon: ########################################
2021-06-02 14:14:57 [58e2ccd64857] 2021.06.02 14:14:46.237496 [ 107873 ] BaseDaemon: (version 21.7.1.7025, build id: 71855C0BDCBDE2FE0A7AE10830E75CAE24E314AC) (from thread 79221) (query_id: d71c8563-4ebe-43f2-9bf5-b951a275e705) Received signal Unknown signal (-3)
2021-06-02 14:14:57 [58e2ccd64857] 2021.06.02 14:14:46.237539 [ 107873 ] BaseDaemon: Sanitizer trap.

@kssenii
Copy link
Member

kssenii commented Jun 3, 2021

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Jun 3, 2021

Command update: success

Branch has been successfully updated

@kssenii kssenii force-pushed the add-sqlite-support branch 4 times, most recently from 4d3cddd to 4077741 Compare July 11, 2021 17:00
@kssenii kssenii force-pushed the add-sqlite-support branch from 4077741 to d6967db Compare July 11, 2021 19:11
@kssenii kssenii force-pushed the add-sqlite-support branch from f790a56 to bcc77fc Compare July 12, 2021 22:40
@kssenii kssenii marked this pull request as ready for review July 13, 2021 06:24
@kssenii kssenii merged commit a24d2c1 into ClickHouse:master Jul 13, 2021
@alexey-milovidov alexey-milovidov added the bug Confirmed user-visible misbehaviour in official release label Jul 13, 2021
@gyuton
Copy link
Contributor

gyuton commented Jul 15, 2021

Internal documentation ticket: DOCSUP-11655.

@alexey-milovidov alexey-milovidov removed the bug Confirmed user-visible misbehaviour in official release label Jul 25, 2021
@alexey-milovidov
Copy link
Member

@kssenii Need two fixes.

  1. After stress test sometimes server failed to start with the message:
2021.07.25 03:55:07.636901 [ 454892 ] {} <Error> Application: DB::Exception: SQLite database path '/var/lib/clickhouse/user_files/db1' is invalid. Error: No such file or directory: Cannot attach table `test_15`.`sqlite_table3` from metadata file /var/lib/clickhouse/store/eef/eef306b5-da98-46ac-aef3-06b5da9826ac/sqlite_table3.sql from query ATTACH TABLE test_15.sqlite_table3 UUID '195e7c83-8d41-489a-995e-7c838d41b89a' (`col1` String, `col2` Int32) ENGINE = SQLite('/var/lib/clickhouse/user_files/db1', 'table3'): while loading database `test_15` from path /var/lib/clickhouse/metadata/test_15
  1. SQLite tables should allow efficient key-value requests with WHERE key = xxx or WHERE key IN (...) (assuming table is indexed). PS. You said that it does not work but I wonder, why? Simply sending compatible conditions to SQLite query would suffice (see transformQueryForExternalDatabase).

@kssenii
Copy link
Member

kssenii commented Jul 25, 2021

@alexey-milovidov

You said that it does not work but I wonder, why?

It seemed too obvious that it just works (because of transformQueryForExternalDatabase), so I thought you mean something more complicated - for example, StorageEmbededRocksDB has additional code for more efficient processing in case there is a where expression which contains primary key (if this key was specified when creating a clickhouse table with StorageEmbededRocksDB engine).

@alexey-milovidov
Copy link
Member

Good. So, we can already use pre-indexed sqlite tables for efficient key-value lookups.
Another possibility is to allow PRIMARY KEY in table with SQLite engine in ClickHouse and translate it directly to primary key of created sqlite tables.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-feature Pull request with new product feature submodule changed At least one submodule changed in this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants