[#1013] Implement the Database Adapter#1035
Conversation
marcocapozzoli
changed the title
marcocapozzoli
changed the base branch from
masc/1013-database-adapter-cpp-2
to
master
marcocapozzoli
changed the title
- Introduced `load_query_file` method in `ContextLoader` to read SQL queries from a file. - Updated `run` function in `database_adapter_main` to handle multiple queries. - Refactored `AtomPersistenceJob` to include a method for setting producer finished status. - Adjusted batch size in `Pipeline.h` for improved performance. - Added a new Makefile target for running the adapter.
| } | ||
| delete[] children; | ||
| delete value; | ||
| if (value != NULL) { |
There was a problem hiding this comment.
Let's add a TODO here so we can remove this check once we improve HandleTrie::insert() to not accept NULL as value
|
|
||
| bool HandleTrie::exists(const string& key) { | ||
| TrieNode* node = lookup_node(key); | ||
| return node != NULL ? true : false; |
There was a problem hiding this comment.
Suggestion:
return lookup_node(key) != NULL;
src/commons/HandleTrie.cc
Outdated
| #include <iostream> | ||
| #include <stack> | ||
|
|
||
| #include "Logger.h" |
|
|
||
| if (!table.contains("table_name")) { | ||
| LOG_ERROR(msg_base + " missing required key: 'table_name'"); | ||
| has_error = true; |
There was a problem hiding this comment.
Don't we want to break the for loop once validation has failed?
Why we would keep iterating when a single validation failure is not allowed?
There was a problem hiding this comment.
So that the user can check all the errors I may make at once, instead of fixing them one by one.
src/db_adapter/DataMapper.cc
Outdated
| bool BaseSQL2Mapper::insert_handle_if_missing(const string& handle) { | ||
| auto exists = this->handle_trie.exists(handle); | ||
| if (exists) return false; | ||
| this->handle_trie.insert(handle, NULL); |
There was a problem hiding this comment.
I think we don't want to allow NULL here...
We should use the same approach that I used at the RemoteAtomDBPeer.h (the EmptyTrieValue).
Maybe we should move the EmptyTrieValue class to commons and use it here
| int port; | ||
|
|
||
| private: | ||
| bool connected; |
There was a problem hiding this comment.
Doesn't this need to me atomic?
std::atomic<bool> connected
| bool second_level = false) override; | ||
| void map_sql_query(const string& virtual_name, const string& raw_query) override; | ||
|
|
||
| mutex api_mutex; |
There was a problem hiding this comment.
Does this need to be public?
| for (const auto& table : tables) { | ||
| if (table.name == name) return table; | ||
| } | ||
| Utils::error("Table '" + name + "' not found in the database."); |
There was a problem hiding this comment.
I think it would be better to not throw here, just log the error.
|
|
||
| conn->stop(); | ||
|
|
||
| EXPECT_FALSE(conn->is_connected()); |
There was a problem hiding this comment.
Let's add a delete conn here and in the catch so we can release the connection.
src/tests/cpp/db_adapter_test.cc
Outdated
|
|
||
| vector<Table> tables_cached = wrapper->list_tables(); | ||
|
|
||
| ASSERT_EQ(tables_cached.size(), tables_cached.size()); |
There was a problem hiding this comment.
ASSERT_EQ(tables_cached.size(), tables.size())
| using namespace db_adapter; | ||
| using namespace processor; | ||
|
|
||
| void ctrl_c_handler(int) { |
There was a problem hiding this comment.
As we are dealing with DBs, it would be better to be sure all operations were done before exit(0) so we don't corrupt any data.
[#1013] Build scripts
…cleanup in database classes
4 participants
This PR adds Processors to the database connection. i.e, implement commons/processor/Processor.h
Resolves #1013