results removal #270

OrangeSmokingJacket · 2023-11-28T12:12:21Z

Functionality of components::result::result_t has been moved to components::cursor::cursor_t.
dispatcher_t and other associated structures now return and receive cursor_t_ptr, which can carry operation status(success or failure), error with description (if there was one) and changes to a collection

ctests and pytests are working properly

SergeiNA

This is more like cosmetic notes (except explicit for ctor). Also could you plz add describtion into your PR? Looks like we just moved from multiple result types to one cursor type.

components/cursor/cursor.hpp

components/cursor/cursor.cpp

components/cursor/cursor.hpp

components/cursor/cursor.cpp

SergeiNA · 2023-12-04T02:10:36Z

Looks fine to me, but wait @kotbegemot approve plz.

SergeiNA · 2023-12-04T02:26:22Z

services/memory_storage/memory_storage.cpp

@@ -124,7 +124,7 @@ namespace services {
    bool memory_storage_t::check_database_(components::session::session_id_t& session, const database_name_t& name) {
        if (!is_exists_database_(name)) {
            actor_zeta::send(current_message()->sender(), this->address(), handler_id(route::execute_plan_finish), session,
-                             make_error(error_code_t::database_not_exists, "database not exists"));
+                            make_error(actor_zeta::detail::pmr::get_default_resource(), error_code_t::database_not_exists, "database not exists"));


May be we can use namespace alias here? @kotbegemot WDYT?
namespace az_res = actor_zeta::detail::pmr

I saw this actor_zeta::detail::pmr::get_default_resource() @kotbegemot why don't just wrap this function and place in in some memory utils?

For example in core::pmr?

std::pmr::memory_resource* default_resource(){ return actor_zeta::detail::pmr::get_default_resource(); }

and usage:

make_error(core::pmr::get_default_resource(), error_code_t::database_not_exists, "database not exists"));

I don't like an idea of usage function from detail namespace of other library. Usually this namespace means "for internal usage".

for now please use
https://en.cppreference.com/w/cpp/memory/get_default_resource

Even though, I thinl it'll better to create aliase fot this, if we want in a future chanege it to something else it will be much easier to do in one place.

SergeiNA · 2023-12-04T02:31:44Z

services/dispatcher/dispatcher.cpp

@@ -211,7 +210,7 @@ namespace services::dispatcher {
            }

            if (ql->type() == statement_type::create_collection) {
-                collection_address_book_.emplace(collection_full_name_t(ql->database_, ql->collection_), result.get<result_address_t>().address);
+                collection_address_book_.emplace(collection_full_name_t(ql->database_, ql->collection_), result->begin()->get()->address());


result->begin()->get()->address() @kotbegemot is it expected? I mean result->begin() looks quite strange to me and not logical at first glance.

It's better to cover in some method and just call
result->get_address();

SergeiNA · 2023-12-04T03:39:43Z

@OrangeSmokingJacket @kotbegemot What is selection logic to use
make_error();
or
make_cursor(..., operation_status_t::failure));
On what stage we start to use make_cursor(..., operation_status_t::failure));?

SergeiNA · 2023-12-04T02:49:19Z

services/dispatcher/dispatcher.cpp

            remove_session(session_to_address_, session);
        }
+        actor_zeta::send(s.address(), dispatcher_t::address(), handler_id(route::execute_ql_finish), session, result);


@OrangeSmokingJacket please pay attention while rebase on the latest commit with inserts. This operation should be valid only for non mutable or from wal operations.

@makeyavelly cc

SergeiNA · 2023-12-04T03:00:09Z

services/memory_storage/memory_storage.cpp

@@ -124,7 +124,7 @@ namespace services {
    bool memory_storage_t::check_database_(components::session::session_id_t& session, const database_name_t& name) {
        if (!is_exists_database_(name)) {
            actor_zeta::send(current_message()->sender(), this->address(), handler_id(route::execute_plan_finish), session,
-                             make_error(error_code_t::database_not_exists, "database not exists"));
+                            make_error(actor_zeta::detail::pmr::get_default_resource(), error_code_t::database_not_exists, "database not exists"));


I saw this actor_zeta::detail::pmr::get_default_resource() @kotbegemot why don't just wrap this function and place in in some memory utils?

For example in core::pmr?

std::pmr::memory_resource* default_resource(){ return actor_zeta::detail::pmr::get_default_resource(); }

and usage:

make_error(core::pmr::get_default_resource(), error_code_t::database_not_exists, "database not exists"));

I don't like an idea of usage function from detail namespace of other library. Usually this namespace means "for internal usage".

SergeiNA · 2023-12-04T03:02:48Z

services/dispatcher/dispatcher.cpp

@@ -211,7 +210,7 @@ namespace services::dispatcher {
            }

            if (ql->type() == statement_type::create_collection) {
-                collection_address_book_.emplace(collection_full_name_t(ql->database_, ql->collection_), result.get<result_address_t>().address);
+                collection_address_book_.emplace(collection_full_name_t(ql->database_, ql->collection_), result->begin()->get()->address());


It's better to cover in some method and just call
result->get_address();

SergeiNA · 2023-12-04T03:05:51Z

services/dispatcher/dispatcher.cpp

@@ -263,11 +262,12 @@ namespace services::dispatcher {
            auto logic_plan = create_logic_plan(statement).first;
            actor_zeta::send(it_collection->second, dispatcher_t::address(), collection::handler_id(collection::route::insert_documents), session, logic_plan, components::ql::storage_parameters{});
        } else {
-            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::insert_finish), session, result_insert(resource_));
+            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::insert_finish), session,
+                             make_from_sub_cursor(actor_zeta::detail::pmr::get_default_resource(), new sub_cursor_t(resource_, address)));


This code hard to read, looks like 2 lines, lets create cursor before send and move it:

auto cursor = make_from_sub_cursor(actor_zeta::detail::pmr::get_default_resource(), new sub_cursor_t(resource_, address)); actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::insert_finish), session, std::move(cursor));

and you can do this for all similar cases.
@kotbegemot What is your opinion?

The dispatcher does not create cursors; the logic needs to be changed.

@kotbegemot you mean this line is not correct?

actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::insert_finish), session, make_from_sub_cursor(actor_zeta::detail::pmr::get_default_resource(), new sub_cursor_t(resource_, address)));

SergeiNA · 2023-12-04T03:06:17Z

services/dispatcher/dispatcher.cpp

@@ -298,11 +298,12 @@ namespace services::dispatcher {
            auto logic_plan = create_logic_plan(statement);
            actor_zeta::send(it_collection->second, dispatcher_t::address(), collection::handler_id(collection::route::delete_documents), session, std::move(logic_plan.first), std::move(logic_plan.second));
        } else {
-            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::delete_finish), session, result_delete(resource_));
+            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::delete_finish), session,


SergeiNA · 2023-12-04T03:06:26Z

services/dispatcher/dispatcher.cpp

@@ -333,11 +334,12 @@ namespace services::dispatcher {
            auto logic_plan = create_logic_plan(statement);
            actor_zeta::send(it_collection->second, dispatcher_t::address(), collection::handler_id(collection::route::update_documents), session, std::move(logic_plan.first), std::move(logic_plan.second));
        } else {
-            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::update_finish), session, result_update(resource_));
+            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::update_finish), session,
+                             make_from_sub_cursor(actor_zeta::detail::pmr::get_default_resource(), new sub_cursor_t(resource_, address)));


SergeiNA · 2023-12-04T03:09:06Z

services/dispatcher/dispatcher.cpp

@@ -263,11 +262,12 @@ namespace services::dispatcher {
            auto logic_plan = create_logic_plan(statement).first;
            actor_zeta::send(it_collection->second, dispatcher_t::address(), collection::handler_id(collection::route::insert_documents), session, logic_plan, components::ql::storage_parameters{});
        } else {
-            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::insert_finish), session, result_insert(resource_));
+            actor_zeta::send(address, dispatcher_t::address(), collection::handler_id(collection::route::insert_finish), session,
+                             make_from_sub_cursor(actor_zeta::detail::pmr::get_default_resource(), new sub_cursor_t(resource_, address)));


Just a note, no need to fix. @kotbegemot are you ok with new here? if it throw exception, actor_zeta::detail::pmr::get_default_resource() will be released?

The dispatcher does not create cursors; the logic needs to be changed.
for now please use
https://en.cppreference.com/w/cpp/memory/get_default_resource

SergeiNA

In general looks fine, well done!
I have only one question about default resource, why in memory_manegment we use std::prm but in collections actor_zeta::prm::detail

SergeiNA · 2023-12-15T01:07:18Z

services/dispatcher/dispatcher.cpp

            if (ql->type() == statement_type::create_collection) {
                trace(log_, "dispatcher_t::execute_ql_finish: create_collection");
-                collection_address_book_.emplace(collection_full_name_t(ql->database_, ql->collection_), result.get<result_address_t>().address);
+                collection_address_book_.emplace(collection_full_name_t(ql->database_, ql->collection_), last_collection_.at(session));
+                last_collection_.erase(session);


@OrangeSmokingJacket Can you give some comments why we need this?

SergeiNA · 2023-12-15T01:11:06Z

services/memory_storage/memory_storage.cpp


 namespace services {

    using namespace services::memory_storage;

+    std::pmr::memory_resource* default_resource(){


Not bad but I recommend to place it in core directory and use it everywhere instead of std::pmr::get_default_resource()

SergeiNA · 2023-12-15T01:15:19Z

components/cursor/cursor.hpp

+
+    cursor_t_ptr make_cursor(std::pmr::memory_resource* resource, operation_status_t op_status);
+    cursor_t_ptr make_cursor(std::pmr::memory_resource* resource);
+    cursor_t_ptr make_cursor(std::pmr::memory_resource* resource, error_code_t type, const std::string& what = std::string());


@OrangeSmokingJacket @kotbegemot this function designed only for error cursor, so might be we can rename it into
make_error_cursor
or
make_error
in this case much easier to read code when we return an error.

SergeiNA · 2023-12-15T01:17:24Z

services/collection/create_index.cpp

@@ -44,7 +44,8 @@ namespace services::collection {
    void collection_t::create_index(const session_id_t& session, create_index_t& index) {
        debug(log(), "collection::create_index : {} {} {}", name_.to_string(), name_index_type(index.index_type_), keys_index(index.keys_)); //todo: maybe delete
        if (dropped_) {
-            actor_zeta::send(current_message()->sender(), address(), handler_id(route::create_index_finish), session, result_create_index(false));
+            actor_zeta::send(current_message()->sender(), address(), handler_id(route::create_index_finish), session,
+                             make_cursor(actor_zeta::detail::pmr::get_default_resource(), error_code_t::collection_dropped));


@OrangeSmokingJacket @kotbegemot sorry I don't get, for actors we use actor_zeta::detail::pmr::get_default_resource() in make_cursor ? Not std::prm::get_default_resource() ?

SergeiNA · 2023-12-15T01:19:17Z

services/memory_storage/memory_storage.cpp

@@ -132,7 +136,7 @@ namespace services {
        if (check_database_(session, name.database)) {
            if (!is_exists_collection_(name)) {
                actor_zeta::send(current_message()->sender(), this->address(), handler_id(route::execute_plan_finish), session,
-                                 make_error(error_code_t::collection_not_exists, "collection not exists"));
+                                make_cursor(default_resource(), error_code_t::collection_not_exists, "collection not exists"));


In collection we still use default_resource() from actor_zeta, why?

SergeiNA · 2023-12-15T01:22:16Z

services/collection/collection.cpp

+            actor_zeta::send(dispatcher, address(), handler_id(route::size_finish), session,
+                             make_cursor(context_->resource(), error_code_t::collection_dropped));
+        } else {
+            auto* sub_cursor = new sub_cursor_t(context_->resource(), address());


@kotbegemot This place(how we add docs into cursor) looks like can be improvement in the future. For now we can leave it as it is.

SergeiNA · 2023-12-15T19:05:18Z

components/cursor/tests/test_cursor.cpp

        REQUIRE(cursor->is_success());
        REQUIRE(!cursor->is_error());
    }
    INFO("error cursor") {
        std::string description = "error description";
-        auto cursor = components::cursor::make_cursor(actor_zeta::detail::pmr::get_default_resource(), components::cursor::error_code_t::other_error, description);
+        auto cursor = components::cursor::make_cursor(std::pmr::get_default_resource(), components::cursor::error_code_t::other_error, description);


use default_resource() function

results removal intermediate

d664b8d

kotbegemot assigned OrangeSmokingJacket Nov 28, 2023

results removal WIP

b75be69

SergeiNA reviewed Nov 29, 2023

View reviewed changes

kotbegemot changed the title ~~results removal WIP~~ results removal Nov 29, 2023

removed results

65b890a

kotbegemot reviewed Nov 29, 2023

View reviewed changes

components/cursor/cursor.hpp Outdated Show resolved Hide resolved

components/cursor/cursor.cpp Outdated Show resolved Hide resolved

OrangeSmokingJacket added 2 commits November 29, 2023 23:54

results removal

ec2c851

results removal

45d0ed6

kotbegemot mentioned this pull request Nov 30, 2023

refactoring: Substitute result_t/insert/delete/e.t.c with cursor_t #272

Closed

kotbegemot linked an issue Nov 30, 2023 that may be closed by this pull request

refactoring: Substitute result_t/insert/delete/e.t.c with cursor_t #272

Closed

SergeiNA reviewed Dec 4, 2023

View reviewed changes

OrangeSmokingJacket added 9 commits December 5, 2023 17:49

results removal; pytest fix

66f4cbc

minor changes and some suggestions implementation

ff01ee9

Merge remote-tracking branch 'upstream/develop' into develop

1d8994e

results removal

67ecaeb

results removal, with duckstax#276 integrated

2e96a9f

results removal, with duckstax#276 integration

0ada18f

results removal

3b65b71

results removal

8fbf302

results removal

05598c4

SergeiNA reviewed Dec 15, 2023

View reviewed changes

results removal

806a87a

SergeiNA reviewed Dec 15, 2023

View reviewed changes

SergeiNA self-requested a review December 15, 2023 19:07

SergeiNA approved these changes Dec 15, 2023

View reviewed changes

results removal

7be9970

kotbegemot approved these changes Dec 15, 2023

View reviewed changes

kotbegemot merged commit 10f54ab into duckstax:develop Dec 15, 2023
16 checks passed

kotbegemot added enhancement New feature or request refactoring labels Dec 15, 2023

OrangeSmokingJacket mentioned this pull request Jan 5, 2024

CSharp integration #299

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

results removal #270

results removal #270

OrangeSmokingJacket commented Nov 28, 2023 •

edited

SergeiNA left a comment

SergeiNA commented Dec 4, 2023 •

edited

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

kotbegemot Dec 12, 2023

SergeiNA Dec 12, 2023

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

SergeiNA commented Dec 4, 2023

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

kotbegemot Dec 12, 2023

SergeiNA Dec 12, 2023

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

SergeiNA Dec 4, 2023

kotbegemot Dec 12, 2023

SergeiNA left a comment

SergeiNA Dec 15, 2023

SergeiNA Dec 15, 2023

SergeiNA Dec 15, 2023

SergeiNA Dec 15, 2023

SergeiNA Dec 15, 2023

SergeiNA Dec 15, 2023

SergeiNA Dec 15, 2023

results removal #270

results removal #270

Conversation

OrangeSmokingJacket commented Nov 28, 2023 • edited

SergeiNA left a comment

Choose a reason for hiding this comment

SergeiNA commented Dec 4, 2023 • edited

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

SergeiNA commented Dec 4, 2023

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

SergeiNA 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

OrangeSmokingJacket commented Nov 28, 2023 •

edited

SergeiNA commented Dec 4, 2023 •

edited