Move key_begin and key_end members from scan_context to scan_info #127

YoshiakiNishimura · 2024-10-22T09:31:46Z

https://github.com/project-tsurugi/tsurugi-issues/issues/991

#124 の続き

scan_context

以下のメンバーを取り除く
data::aligned_buffer key_begin_{};
data::aligned_buffer key_end_{};

scan_info

以下のメンバーを追加する
data::aligned_buffer key_begin_{};
data::aligned_buffer key_end_{};
std::size_t blen_{};
std::size_t elen_{};
status status_result_{};
std::unique_ptr<memory_resource> varlen_resource_{};

operator_builder::operator

配下で以下の設定を行う
data::aligned_buffer key_begin_{};
data::aligned_buffer key_end_{};
std::size_t blen_{};
std::size_t elen_{};
status status_result_{};
std::unique_ptr<memory_resource> varlen_resource_{};

scan::open

operator_builder::operatorに設定を移動するのでそれ以外の設定を残す

YoshiakiNishimura · 2024-10-22T10:01:12Z

@kuron99

scan_testまで貫通いたしましたのでレビューお願いいたします

kuron99 · 2024-10-23T06:45:48Z

src/jogasaki/data/aligned_buffer.h

@@ -162,6 +162,7 @@ class aligned_buffer {
     * @return the output
     */
    friend std::ostream& operator<<(std::ostream& out, aligned_buffer const& value);
+    void dump(std::ostream& out, int indent = 0) const noexcept;


doc commentを追加してください。あと、friend関数の上に移動させてください。

kuron99 · 2024-10-23T06:50:36Z

src/jogasaki/executor/process/impl/processor.cpp

@@ -43,7 +43,7 @@ processor::processor(
    std::shared_ptr<ops::io_info> io_info,
    std::shared_ptr<relation_io_map> relation_io_map,
    io_exchange_map& io_exchange_map,
-    memory::lifo_paged_memory_resource* resource
+    request_context* resource


resource という名前も変更してください。

kuron99 · 2024-10-23T07:15:27Z

test/jogasaki/operator_test_utils.h

@@ -176,6 +176,7 @@ class operator_test_utils {
    takatori::plan::process& process_;  //NOLINT

    memory::page_pool pool_{};  //NOLINT
+    request_context request_context_{}; //NOLINT


これって必要なのでしょうか？どこかで使っています？

必要です
改造でscan::openでiterator関連の設定をしなくなった関係でscan_test.cppを通すためにscan_info::encode_key関数呼び出しが必要になりました。その関数の引数としてrequest_context_が必要です。
その代わり従来のmemory::lifo_paged_memory_resource resource_; //NOLINTが不要になっていましたがこちらは消し忘れなのでresource_は消します

了解。使っているのであればOKです。

kuron99 · 2024-10-23T07:25:07Z

src/jogasaki/executor/process/impl/task_context.cpp

@@ -123,6 +123,9 @@ io::record_writer* task_context::external_writer() {
 class abstract::scan_info const* task_context::scan_info() {
    return scan_info_.get();
 }
+std::shared_ptr<impl::scan_info> task_context::non_const_impl_scan_info() {


関数名が少し仰々しいので shared_scan_info あたりでどうでしょうか。
返り値の型も std::shared_ptr<impl::scan_info> const& にしてもらえますか。(コピーが不要な場合のために)

kuron99 · 2024-10-23T07:28:18Z

src/jogasaki/executor/process/impl/ops/operator_builder.cpp

@@ -132,7 +130,7 @@ std::unique_ptr<operator_base> operator_builder::operator()(const relation::scan
    // scan info is not passed to scan operator here, but passed back through task_context
    // in order to support parallel scan in the future
    scan_info_ = create_scan_info(node, secondary_or_primary_index);


create_scan_infoにrequest_context_を渡してエンコードまでしてしまうほうがシンプルではないでしょうか。
scan_infoのbegin_columns_, end_columns_メンバはもう不要で除去してしまっていいように思います。

了解しました。

kuron99

ざっくりとですが、レビューしてコメントしました。

YoshiakiNishimura · 2024-10-24T09:52:50Z

@kuron99 対応しました

kuron99 · 2024-10-25T08:13:44Z

src/jogasaki/executor/process/impl/ops/operator_builder.h

@@ -90,7 +90,7 @@ class operator_builder {
        std::shared_ptr<io_info> io_info,
        std::shared_ptr<relation_io_map> relation_io_map,
        io_exchange_map& io_exchange_map,
-        memory::lifo_paged_memory_resource* resource = nullptr
+        request_context* request_context = nullptr


doc commentの @param部分も一緒に修正お願いします。

kuron99 · 2024-10-25T08:23:26Z

src/jogasaki/executor/process/impl/ops/operator_builder.cpp

    );
 }

 std::shared_ptr<impl::scan_info> operator_builder::create_scan_info(
    relation::scan const& node,
-    yugawara::storage::index const& index
+    yugawara::storage::index const& index,
+    std::unique_ptr<ops::context_base::memory_resource> varlen_resource,


ここはrequest_contextのrequest_resource()で取れるものを使うというのではダメですか？requestをconstructするのに必要なmemory resourceはあまり大きくないはずなので、この目的のためにglobal poolからアサインするのはもったいない印象です。

@kuron99

scan_infoでbegin_key,end_keyを設定するためにjogasaki::executor::process::impl::ops::details::encode_keyを呼び出していますがこれの第一引数が request_context* contextなのでencode_keyを改造する必要があります。改造しますか？

expr::evaluator_context ctx{ std::addressof(resource), context ? utils::make_function_context(*context->transaction()) : nullptr, };

global poolにアサインしているのはこれまでの挙動を変えたくないからで flow::create_task_context下記コードに引っ張らています。

ctx->work_context( std::make_unique<impl::work_context>( context_, operators.size(), info_->vars_info_list().size(), std::make_unique<memory::lifo_paged_memory_resource>(&global::page_pool()), std::make_unique<memory::lifo_paged_memory_resource>(&global::page_pool()), context_->database(), context_->transaction(), empty_input_from_shuffle_ ) );

今回ここは下記の通り、これまでと同じ状態を保つために少し面倒な改造を行っています。

auto scan = ctx->shared_scan_info(); ctx->work_context( std::make_unique<impl::work_context>( context_, operators.size(), info_->vars_info_list().size(), std::make_unique<memory::lifo_paged_memory_resource>(&global::page_pool()), (scan == nullptr)?std::make_unique<memory::lifo_paged_memory_resource>(&global::page_pool()):scan->varlen_resource(), context_->database(), context_->transaction(), empty_input_from_shuffle_ ) );

scan_infoでbegin_key,end_keyを設定するためにjogasaki::executor::process::impl::ops::details::encode_keyを呼び出していますがこれの第一引数が request_context* contextなのでencode_keyを改造する必要があります。改造しますか？

request_contextがGODオブジェクト化していてよくないのですが、いまのところ必要な場所にrequest_contextを渡すのはしょうがないかなと思っています。なので、request_contextをoperator_builder等にわたす点については問題ありません。

global poolにアサインしているのはこれまでの挙動を変えたくないからで flow::create_task_context下記コードに引っ張らています。

うーん、なるほど、work_contextへmake_uniqueして渡しているのがタイミングが変わったので移動してきた感じですか。ただ一旦scan_infoへ渡して、引き剥がしてimpl::work_contextへ渡すのが迂遠な感じはありますね。

@YoshiakiNishimura work_contextのmemory resourceを使う必要はないと思うので、scan_infoを作成するのに使う必要がある場合は request_contextのrequest_resource() を使う、という方針にしましょう。

既存のコードはwork_contextが持つvarlen resourceを使っていました。これはprocess内のオペレータが並列化された際、その並列化単位の1個1個にアサインされたvarlen resourceで、並列で動く(可能性のある) scanの内部でエンコードを行うのでこれを使うしかありませんでした。
一方これからはscanの外部で並列化前にシングルスレッドで処理される部分(create_tasksやcreate_scan_info, operator_builderなど)でscan_infoを作ることになるので、ここではwork_contextのものを使う必要はなく、リクエスト全体で持っている(request_context::request_resource()で取得できる)ものをつかっていいと思います。

kuron99 · 2024-10-25T09:29:10Z

src/jogasaki/executor/process/impl/scan_info.h

    /**
     * @brief create new object
     */
    explicit scan_info(
-        std::vector<ops::details::search_key_field_info> = {},
+        std::vector<ops::details::search_key_field_info> const& begin_columns = {},


このPRでなくてもいいのですが、下記のような形でstd::string_view経由でキーを渡すコンストラクタが必要になりそうなので追加しておいてもいいかもしれません。(スキャンするキー区間を外部で作成した際にscan_infoを作る用途)
std::vectorops::details::search_key_field_infoを受け取るコンストラクタはstd::string_view経由のコンストラクタに委譲できるとキレイですが、コンストラクタ内部で複数の変数にアクセスしたりすると難しいかもしれません。その場合は別々のコンストラクタでもいいと思います。

explicit scan_info( std::string_view key_begin = {}, kvs::end_point_kind begin_endpoint = kvs::end_point_kind::unbound, std::string_view key_end = {}, kvs::end_point_kind end_endpoint = kvs::end_point_kind::unbound, std::unique_ptr<memory_resource> varlen_resource = nullptr, request_context* request_context = nullptr );

上記について、もう少し細かく考えました。

「scan_infoは純粋にスキャン区間をあらわすものとする」
「スキャン区間をstd::vector<search_key_field_info>で表していたのはやめ、(data::aligned_bufferを使って保存される) std::string_viewで表す」

というポリシーとし、scan外部でscan_infoを作成しやすくするべきと考えます。既存のコードはindexの構造に密になっていたのでsearch_key_field_infoでよかったのですが、今後は外部でキー分布等の情報だけからscan_infoを作ることになるのでなるべく余計な依存を持たず、純粋に区間を表すキーだけを保持するのがいいと思います。

そのため、エンコードしてキーを作る、という部分はscan_infoから分離してcreate_scan_info等に移動しましょう。そうすれば、コンストラクタはvarlen_resourceやrequest_contextを受け取らなくてよくなります。

エンコード中にエラーが起きた際にstatus_result_に詰めてscan実行時にチェックするというロジックもやめて、エンコードした部分からエラーを戻すようにしましょう。
processorのコンストラクタやoperator_builder内部で構造を構築している際にエラーが起きたときのエラー処理としてはimpl::compiler_exceptionを投げる、というやり方が可能です。下記のコードを参考にしてください。

jogasaki/src/jogasaki/executor/process/impl/ops/write_existing.cpp

Lines 411 to 412 in 1d1e828

throw_exception(plan::impl::compile_exception{

create_error_info(error_code::restricted_operation_exception, msg, status::err_illegal_operation)

kuron99 · 2024-10-28T02:12:27Z

src/jogasaki/executor/process/impl/ops/operator_builder.cpp

-    yugawara::storage::index const& index
+    yugawara::storage::index const& index,
+    std::unique_ptr<ops::context_base::memory_resource> varlen_resource,
+    request_context* request_context
 ) {
    return std::make_shared<impl::scan_info>(


別のコメントでも述べましたが、この位置にエンコード処理を持ってくることを検討してもらえませんか。エンコードで使うためにここまではrequest_contextが必要ですが、scan_infoのコンストラクタにはrequest_context, varlen_resourceを渡さなくてすむと思っています。

@kuron99
これは最初のようにscan_info->encode_key(request_context)という感じでエンコード処理を分離するという認識であってます？

いえ、scan_infoからもエンコード処理を分離することを意図しています。scan_infoの外側でエンコードを行い、scan_infoは出来上がったものを受け取るだけにする形です。

kuron99

コメントしました。レビュー完了で一旦戻します。

YoshiakiNishimura marked this pull request as ready for review October 22, 2024 10:00

YoshiakiNishimura requested a review from kuron99 as a code owner October 22, 2024 10:00

kuron99 reviewed Oct 23, 2024

View reviewed changes

kuron99 requested changes Oct 23, 2024

View reviewed changes

YoshiakiNishimura force-pushed the move-key-to-scan-info3 branch from 648b10e to 1425b34 Compare October 23, 2024 10:17

YoshiakiNishimura marked this pull request as draft October 23, 2024 10:18

YoshiakiNishimura force-pushed the move-key-to-scan-info3 branch 6 times, most recently from df4165d to 0384c68 Compare October 24, 2024 08:35

Move key_begin and key_end members from scan_context to scan_info

1d1e828

YoshiakiNishimura force-pushed the move-key-to-scan-info3 branch from 0384c68 to 1d1e828 Compare October 24, 2024 09:30

YoshiakiNishimura marked this pull request as ready for review October 24, 2024 09:52

kuron99 reviewed Oct 25, 2024

View reviewed changes

kuron99 reviewed Oct 28, 2024

View reviewed changes

kuron99 requested changes Oct 28, 2024

View reviewed changes

YoshiakiNishimura mentioned this pull request Oct 31, 2024

Replaced scan_info definition with the range class #129

Merged

YoshiakiNishimura closed this Nov 13, 2024

YoshiakiNishimura deleted the move-key-to-scan-info3 branch November 13, 2024 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move key_begin and key_end members from scan_context to scan_info #127

Move key_begin and key_end members from scan_context to scan_info #127

YoshiakiNishimura commented Oct 22, 2024 •

edited

Loading

YoshiakiNishimura commented Oct 22, 2024

kuron99 Oct 23, 2024

kuron99 Oct 23, 2024

kuron99 Oct 23, 2024

YoshiakiNishimura Oct 23, 2024

kuron99 Oct 23, 2024

kuron99 Oct 23, 2024

kuron99 Oct 23, 2024 •

edited

Loading

YoshiakiNishimura Oct 23, 2024

kuron99 left a comment

YoshiakiNishimura commented Oct 24, 2024

kuron99 Oct 25, 2024

kuron99 Oct 25, 2024

YoshiakiNishimura Oct 25, 2024 •

edited

Loading

kuron99 Oct 25, 2024

kuron99 Oct 25, 2024

kuron99 Oct 28, 2024

kuron99 Oct 25, 2024

kuron99 Oct 28, 2024

kuron99 Oct 28, 2024

YoshiakiNishimura Oct 28, 2024

kuron99 Oct 28, 2024 •

edited

Loading

kuron99 left a comment

	throw_exception(plan::impl::compile_exception{
	create_error_info(error_code::restricted_operation_exception, msg, status::err_illegal_operation)

Move key_begin and key_end members from scan_context to scan_info #127

Move key_begin and key_end members from scan_context to scan_info #127

Conversation

YoshiakiNishimura commented Oct 22, 2024 • edited Loading

YoshiakiNishimura commented Oct 22, 2024

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

kuron99 Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kuron99 left a comment

Choose a reason for hiding this comment

YoshiakiNishimura commented Oct 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YoshiakiNishimura Oct 25, 2024 • edited Loading

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

kuron99 Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

kuron99 left a comment

Choose a reason for hiding this comment

YoshiakiNishimura commented Oct 22, 2024 •

edited

Loading

kuron99 Oct 23, 2024 •

edited

Loading

YoshiakiNishimura Oct 25, 2024 •

edited

Loading

kuron99 Oct 28, 2024 •

edited

Loading