MongoDB integration refactoring #63279

allmazz · 2024-05-02T15:11:52Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

MongoDB integration refactored: migration to new driver mongocxx from deprecated Poco::MongoDB, remove support for deprecated old protocol, support for connection by URI, support for all MongoDB types, support for WHERE and ORDER BY statements on MongoDB side, restriction for expression unsupported by MongoDB.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Current MongoDB integration is very limited: not all types are supported; WHERE and ORDER BY conditions applied on ClickHouse side, what needs to read ALL data from MongoDB collection.

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

Modify your CI run

NOTE: If your merge the PR with modified CI you MUST KNOW what you are doing
NOTE: Checked options will be applied if set before CI RunConfig/PrepareRunConfig step

Include tests (required builds will be added automatically):

Exclude tests:

Extra options:

do not test (only style check)
disable merge-commit (no merge from master before tests)
disable CI cache (job reuse)

Only specified batches in multi-batch jobs:

1
2
3
4

alexey-milovidov · 2024-05-04T02:30:25Z

base/harmful/harmful.c

@@ -203,7 +203,8 @@ TRAP(lgammal)
 TRAP(nftw)
 TRAP(nl_langinfo)
 TRAP(putc_unlocked)
-TRAP(rand)
+//TRAP(rand) // Used in mongo-c-driver


The driver has to be patched.

The concerns have to be reported to MongoDB authors.

Do you mean mongo-c-driver? Could you please refer to the issue, is it small patch?

How did you handle it?

contrib/mongo-c-driver-cmake/CMakeLists.txt

robot-clickhouse-ci-2 · 2024-05-04T02:31:55Z

This is an automated comment for commit eb6713d with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
A Sync	If it fails, ask a maintainer for help	⏳ pending
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	❌ failure
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	⏳ pending
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	❌ error
Mergeable Check	Checks if all other necessary checks are successful	❌ failure

Successful checks

Check name	Description	Status
PR Check	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success

vdimir · 2024-05-06T15:01:01Z

Great initiative!

A couple of questions for now:

Does it support the mongodb+srv:// protocol? Can it be used directly with Atlas?
Are the connection parameters supported by the current Poco::MongoDB implementation? A user may pass something like authSource=admin;ssl=true;readPreference=primaryPreferred;retryWrites=true;connectTimeoutMS=5000;socketTimeoutMS=5000 as the last storage argument, and some of these parameters are parsed by Poco. Won't this be broken?

allmazz · 2024-05-06T15:11:46Z

Does it support the mongodb+srv:// protocol? Can it be used directly with Atlas?

It's supposed, but we have to test it

allmazz · 2024-05-06T15:12:43Z

Won't this be broken?

should not, but need to be tested.
I will check it out soon

allmazz · 2024-05-12T18:02:59Z

Does it support the mongodb+srv:// protocol? Can it be used directly with Atlas?

It's supposed, but we have to test it

yes, it works now

allmazz · 2024-05-12T18:05:08Z

Won't this be broken?

should not, but need to be tested. I will check it out soon

this will not be broken, but your example is not valid. docs

vdimir

Thank you for the enormous effort you've put into this integration!

Firstly, I've concentrated on high-level aspects of the implementation, and didn't dig into details too much.

I have a few suggestions regarding smoother transition to the new library.
We can maintain the current implementation based on Poco alongside the new one for some time, adding a global setting or configuration option that allows users to toggle between the two. (Additionally, the existing Poco-based implementation could be used with old analyzer, allow_experimental_analyzer = 0)
Source files of the current implementation can be kept appending a suffix like *PocoLegacy.

The new implementation tries to construct queries for MongoDB and throws errors for unsupported queries. Instead we can fallback to reading all data and processes it internally, similar to the existing approach.

Let me know what do you think about these suggestions.

vdimir · 2024-05-27T09:46:06Z

base/harmful/harmful.c

@@ -203,7 +203,8 @@ TRAP(lgammal)
 TRAP(nftw)
 TRAP(nl_langinfo)
 TRAP(putc_unlocked)
-TRAP(rand)
+//TRAP(rand) // Used in mongo-c-driver


How did you handle it?

contrib/mongo-cxx-driver-cmake/CMakeLists.txt

contrib/mongo-c-driver-cmake/CMakeLists.txt

vdimir · 2024-05-28T09:17:44Z

contrib/mongo-c-driver-cmake/CMakeLists.txt

+add_library(ch_contrib::libmongoc ALIAS _libmongoc)
+target_include_directories(_libmongoc SYSTEM PUBLIC ${LIBMONGOC_SOURCE_DIR} ${COMMON_SOURCE_DIR} ${UTF8PROC_SOURCE_DIR})
+target_compile_definitions(_libmongoc PRIVATE MONGOC_COMPILATION)
+target_link_libraries(_libmongoc ch_contrib::libbson ch_contrib::c-ares ch_contrib::zlib resolv)


I don't see any linking with resolv elsewhere (but resolv.h is used in base/poco/Net/src/DNS.cpp ) why it's required explicitly here? Where we will take this library from ?

This is a system library, which should persists in all unix-like systems.
mongo-c-driver needs it here, without explicitly spec build will fail.

vdimir · 2024-05-28T09:26:48Z

docs/en/engines/table-engines/integrations/mongodb.md

+If key not found in MongoDB document, default value or null(if the column is nullable) will be inserted.
+
+## Supported clauses
+*Hint: you can use MongoDB table in CTE to perform any clauses, but be aware, that in some cases, performance will be significantly degraded.*


Do you mean to use

WITH (SELECT * FROM mongo_table) as m SELECT ... FROM m GROUP BY ...

So, we read all data mongo_table first and then process the data on ClickHouse side? What will happen when table is used with unsupported clause, will we get a error?

I assume we can calrify this in the documentation here, add an example

Yes, I mean this.
Okay, I will provide some examples.

tests/integration/helpers/cluster.py

src/Dictionaries/MongoDBDictionarySource.cpp

vdimir · 2024-05-28T11:18:02Z

src/Processors/Sources/MongoDBSource.cpp


-                bool exists_in_current_document = document->exists(name);
-                if (!exists_in_current_document)
+                if (sample_column.type->isNullable())


Won't we just insert NULL for Nullable column in insertDefaultValue? Why do we need explicitly set value in nullmap again?

Yes, it will more shorter, thanks.

vdimir · 2024-05-28T11:21:52Z

src/Processors/Sources/MongoDBSource.cpp

+Array BSONArrayAsArray(size_t dimensions, const bsoncxx::types::b_array & array, const DataTypePtr & type, const Field & default_value, const std::string & name)
+{
+    auto arr = Array();
+    if (dimensions > 0)


Should we add some santity check for the number of dimensions ? Probably ClickHouse won't allow to define too nested array, maybe just in case

It's not responsibility of source, is it?
In source we just check data from Mongo, and make sure this can be inserted into given schema.

vdimir · 2024-05-28T11:22:58Z

src/Processors/Sources/MongoDBSource.cpp

+            return json;
+        }
+        case bsoncxx::type::k_binary:
+            return base64Encode(std::string(reinterpret_cast<const char*>(value.get_binary().bytes), value.get_binary().size));


Why we encode it with base64? We can store raw data in clickhouse Strings.
UPD: But it won't work for the case where binary is inside document or array that we serialize to json. How does Mongo return this data with native client when we request document with binary field?

Okay, good idea.

As BSON binary field, the same as when outside document.

allmazz · 2024-05-28T13:05:11Z

Thank you for the enormous effort you've put into this integration!

Firstly, I've concentrated on high-level aspects of the implementation, and didn't dig into details too much.

I have a few suggestions regarding smoother transition to the new library. We can maintain the current implementation based on Poco alongside the new one for some time, adding a global setting or configuration option that allows users to toggle between the two. (Additionally, the existing Poco-based implementation could be used with old analyzer, allow_experimental_analyzer = 0) Source files of the current implementation can be kept appending a suffix like *PocoLegacy.

The new implementation tries to construct queries for MongoDB and throws errors for unsupported queries. Instead we can fallback to reading all data and processes it internally, similar to the existing approach.

Let me know what do you think about these suggestions.

I think we shouldn't support old Poco implementation, just make a settings which control WHERE and ORDER BY behavior. And ignore it when new analyzer used.

rschu1ze · 2024-05-29T19:43:09Z

@allmazz Feel free to throw out the mongodb in Poco once it is unused. ClickHouse maintains its own heavily-patched copy (dump) of Poco in base/poco/ and the less of it exists the smaller the burden.

allmazz · 2024-05-29T20:09:43Z

@allmazz Feel free to throw out the mongodb in Poco once it is unused. ClickHouse maintains its own heavily-patched copy (dump) of Poco in base/poco/ and the less of it exists the smaller the burden.

You mean we shouldn't keep legacy implementation, right?

rschu1ze · 2024-05-29T20:13:22Z

Yes, Poco::MongoDB will no longer be used (I suppose) after your PR, so we can/should get rid of it.

allmazz · 2024-05-29T20:15:06Z

Yes, Poco::MongoDB will no longer be used (I suppose) after your PR, so we can/should get rid of it.

@vdimir thinks we should keep old implementation too, so it will be deleted later.

CLAassistant · 2024-06-02T00:03:43Z

All committers have signed the CLA.

…ailure, fix ssl

allmazz added 5 commits April 20, 2024 21:22

add libs

767ee46

WIP

5a25ba7

WIP

400af8d

rm StorageMongoDBSocketFactory

30b3d08

WIP

fa6bebb

allmazz force-pushed the mongodb_refactoring branch from a46d70b to fa6bebb Compare May 2, 2024 16:50

Merge branch 'master' into mongodb_refactoring

8e2adaa

alexey-milovidov added the can be tested Allows running workflows for external contributors label May 4, 2024

alexey-milovidov reviewed May 4, 2024

View reviewed changes

contrib/mongo-c-driver-cmake/CMakeLists.txt Outdated Show resolved Hide resolved

robot-clickhouse-ci-2 added the pr-feature Pull request with new product feature label May 4, 2024

alexey-milovidov removed the can be tested Allows running workflows for external contributors label May 4, 2024

robot-clickhouse-ci-2 added the submodule changed At least one submodule changed in this PR. label May 4, 2024

vdimir self-assigned this May 6, 2024

URI as engine parameter

e6f36aa

alexey-milovidov added the can be tested Allows running workflows for external contributors label May 12, 2024

allmazz added 3 commits May 16, 2024 23:35

WIP, arrays support

0887962

Merge remote-tracking branch 'origin/master' into mongodb_refactoring

eadd680

WIP

a4117b5

vdimir marked this pull request as draft May 21, 2024 09:52

allmazz added 3 commits May 26, 2024 01:56

WIP

f07fc72

fix typo in docs

2e898be

Merge branch 'master' into mongodb_refactoring

25f7049

allmazz marked this pull request as ready for review May 25, 2024 23:06

allmazz changed the title ~~MongoDB integration refactoring: WIP~~ MongoDB integration refactoring May 25, 2024

new test cases, some fixes

baa329b

allmazz requested a review from vdimir May 26, 2024 17:53

allmazz added 2 commits May 26, 2024 21:11

fix CS

b532e7c

fix build on freebsd, documentation for mongodb table func

d4b7bac

vdimir reviewed May 28, 2024

View reviewed changes

allmazz force-pushed the mongodb_refactoring branch 2 times, most recently from 86c3296 to 06b0289 Compare June 2, 2024 00:25

allmazz added 2 commits June 2, 2024 03:28

Merge branch 'master' into HEAD

4f5fdb4

possibility to use the legacy impl, settings for ignore query build f…

eb6713d

…ailure, fix ssl

allmazz force-pushed the mongodb_refactoring branch from 06b0289 to eb6713d Compare June 2, 2024 00:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MongoDB integration refactoring #63279

MongoDB integration refactoring #63279

allmazz commented May 2, 2024 •

edited

alexey-milovidov May 4, 2024

alexey-milovidov May 4, 2024

vdimir May 6, 2024

vdimir May 27, 2024

robot-clickhouse-ci-2 commented May 4, 2024 •

edited by robot-ch-test-poll1

vdimir commented May 6, 2024

allmazz commented May 6, 2024

allmazz commented May 6, 2024

allmazz commented May 12, 2024

allmazz commented May 12, 2024

vdimir left a comment

vdimir May 27, 2024

vdimir May 28, 2024

allmazz May 28, 2024

vdimir May 28, 2024

allmazz May 28, 2024

vdimir May 28, 2024

allmazz May 28, 2024

vdimir May 28, 2024

allmazz May 28, 2024

vdimir May 28, 2024

allmazz May 28, 2024

allmazz commented May 28, 2024 •

edited

rschu1ze commented May 29, 2024

allmazz commented May 29, 2024

rschu1ze commented May 29, 2024

allmazz commented May 29, 2024

CLAassistant commented Jun 2, 2024 •

edited

MongoDB integration refactoring #63279

Are you sure you want to change the base?

MongoDB integration refactoring #63279

Conversation

allmazz commented May 2, 2024 • edited

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

Include tests (required builds will be added automatically):

Exclude tests:

Extra options:

Only specified batches in multi-batch jobs:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robot-clickhouse-ci-2 commented May 4, 2024 • edited by robot-ch-test-poll1

vdimir commented May 6, 2024

allmazz commented May 6, 2024

allmazz commented May 6, 2024

allmazz commented May 12, 2024

allmazz commented May 12, 2024

vdimir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allmazz commented May 28, 2024 • edited

rschu1ze commented May 29, 2024

allmazz commented May 29, 2024

rschu1ze commented May 29, 2024

allmazz commented May 29, 2024

CLAassistant commented Jun 2, 2024 • edited

allmazz commented May 2, 2024 •

edited

robot-clickhouse-ci-2 commented May 4, 2024 •

edited by robot-ch-test-poll1

allmazz commented May 28, 2024 •

edited

CLAassistant commented Jun 2, 2024 •

edited