Replaced scan_info definition with the range class #129

YoshiakiNishimura · 2024-10-31T07:03:57Z

#127 の続き

YoshiakiNishimura · 2024-10-31T07:55:39Z

@kuron99 scan_infoをrangeに書き換えました。まだtestを通ってませんのでtestが通るように随時調整していきます

YoshiakiNishimura · 2024-10-31T16:49:05Z

テスト失敗メモ

場所

api_test.scan_with_host_variable

fail_log.txt

Expected equality of these values:
  ""
    Which is: 0x560488566cbb
  builder() .text(query) .params(params) .vars(variables) .output_records(out) .run() .report()
    Which is: "execution failed. executor::execute() - err_integrity_constraint_violation"

/home/nishimura/git/tsurugidb/jogasaki/test/jogasaki/api/api_test_base.cpp:158: Failure
Expected equality of these values:
  ""
    Which is: 0x560488566cbb                                                                                    builder() .text(query) .params(params) .vars(variables) .output_records(out) .run() .report()
    Which is: "execution failed. executor::execute() - err_integrity_constraint_violation"

テスト考察

実際のQueryは通るのでテスト固有の問題

tgsql> CREATE TABLE T0 (C0 INT,C1 FLOAT);
     | INSERT INTO T0 (C0, C1) VALUES (20,20.0);
     | INSERT INTO T0 (C0, C1) VALUES (10,10.0);
     |
     | SELECT * FROM T0 WHERE C0 > 15 AND C0 < 25;
     | SELECT * FROM T0 WHERE C0 > 15 AND C0 < null;
     | SELECT * FROM T0 WHERE C0 > null AND C0 < 15;

[C0: INT4, C1: FLOAT4]
[20, 20.0]
(1 row)

[C0: INT4, C1: FLOAT4]
(0 rows)

[C0: INT4, C1: FLOAT4]
(0 rows)

kuron99 · 2024-11-11T02:18:09Z

@YoshiakiNishimura 変更前のコードだと下記部分で「レンジの上端または下端がNULLの場合はnot foundにする」というケアをしているのでこのパスが変更後にどうなったかを調べてもらうといいかもしれません。

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

Line 149 in d984355

if(res == status::err_integrity_constraint_violation) {

YoshiakiNishimura · 2024-11-11T03:40:34Z

@YoshiakiNishimura 変更前のコードだと下記部分で「レンジの上端または下端がNULLの場合はnot foundにする」というケアをしているのでこのパスが変更後にどうなったかを調べてもらうといいかもしれません。

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

Line 149 in d984355

if(res == status::err_integrity_constraint_violation) {

@kuron99

問題の箇所は以下です

jogasaki/src/jogasaki/executor/process/impl/ops/details/encode_key.cpp

Line 79 in d984355

VLOG_LP(log_debug) << "Null assigned for non-nullable field.";

直観として下のコードを書いた場合、上のreturnを通りそうですが、従来はどのような考えで上の処理を通らないようにしていたのですか？

        ps->set_null("p0");
        std::vector<mock::basic_record> result{};
        execute_query("SELECT * FROM T0 WHERE C0 < :p0", variables, *ps, result);

kuron99 · 2024-11-11T04:12:15Z

@YoshiakiNishimura

直観として下のコードを書いた場合、上のreturnを通りそうですが、従来はどのような考えで上の処理を通らないようにしていたのですか？

上のreturnを通っていたと思いますよ。通ったうえでその戻り値 status::err_integrity_constraint_violationが、#129 (comment) のところまで行って、NULLが検知されたことをケアするという動きになっていました。

kuron99 · 2024-11-11T04:19:49Z

考え方としては、INSERT文などでnon-nullableなフィールドにnullをアサインしようとするとNULL制約違反のためencode_keyはstatus::err_integrity_constraint_violationを戻すのですが、encode_keyをscanに使用した場合は条件文の中の話なのでNULL制約違反とはいえなくて「その条件で検索しても何もみつからない」というscanになになります。

厳密にはencode_keyがerr_integrity_constraint_violationを戻すのはやりすぎで「エンコードしようとしたものがNULLだった」ということを返して呼び出し側が判定すべきですが、encode_keyは古いコードでよくないですね。

YoshiakiNishimura · 2024-11-11T06:23:09Z

考え方としては、INSERT文などでnon-nullableなフィールドにnullをアサインしようとするとNULL制約違反のためencode_keyはstatus::err_integrity_constraint_violationを戻すのですが、encode_keyをscanに使用した場合は条件文の中の話なのでNULL制約違反とはいえなくて「その条件で検索しても何もみつからない」というscanになになります。

厳密にはencode_keyがerr_integrity_constraint_violationを戻すのはやりすぎで「エンコードしようとしたものがNULLだった」ということを返して呼び出し側が判定すべきですが、encode_keyは古いコードでよくないですね。

@kuron99

原因わかりました。
今までは
scan::open　のdetails::encode_keyのreturn値がstatus::err_integrity_constraint_violationの場合scan::operatorが

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

Line 152 in d984355

return {};

152行でreturnします。

今回details::encode_keyを前に移動して下記の通り処理してますので152行でreturnしません。

    auto status_result = details::two_encode_keys(request_context_,
        details::create_search_key_fields(secondary_or_primary_index, node.lower().keys(), *info_),
        details::create_search_key_fields(secondary_or_primary_index, node.upper().keys(), *info_),
        vars, *resource_ptr, *key_begin, blen, *key_end, elen);
    if (status_result != status::ok) {
        auto msg = string_builder{} << to_string_view(status_result) << string_builder::to_string;
        throw_exception(jogasaki::plan::impl::compile_exception{create_error_info(
            error_code::sql_execution_exception, msg, status::err_compiler_error)});
    }

今回scan_infoをrangeにしてbegin_,end_だけにしましたが、scan::openにstatus::err_integrity_constraint_violationの情報を伝えるために情報を保持する、statusメンバーを追加いたします。

class range
  private:
    std::unique_ptr<bound> begin_;
    std::unique_ptr<bound> end_;
};

kuron99 · 2024-11-11T08:10:13Z

今回scan_infoをrangeにしてbegin_,end_だけにしましたが、scan::openにstatus::err_integrity_constraint_violationの情報を伝えるために情報を保持する、statusメンバーを追加いたします。

scan結果が空になるということを伝えるためですよね。であれば、statusメンバではなく bool is_empty みたいなプロパティで「空のレンジ」を示すようにするのでどうでしょうか。encode_keyからstatus::err_integrity_constraint_violationから戻された場合は空のレンジを作るようにすると。

YoshiakiNishimura · 2024-11-11T10:40:40Z

@kuron99
対応完了しました

kuron99 · 2024-11-12T00:40:21Z

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

@@ -48,15 +48,15 @@ class processor : public process::abstract::processor {
     * @param io_info input/output information
     * @param relation_io_map mapping from relation to input/output indices
     * @param io_exchange_map map from input/output to exchange operator
-     * @param resource the memory resource to build the structures needed by this processor
+     * @param request_context memory resource for initializing and managing processor structures


渡しているものは request_contextで、コメントの説明は memory resource と言っているのでズレがあります。
あくまで渡すmemory resourceの説明をしたいなら、request_context to pass memory resource ... あたりでしょうか。

kuron99 · 2024-11-12T00:46:09Z

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

@@ -57,15 +57,15 @@ class task_context : public abstract::task_context {
     * @brief create new object
     * @param partition the index of partition assigned to this object (used as index of source on the input exchange)
     * @param io_exchange_map mapping from input/output indices to exchanges
-     * @param scan_info the scan information, nullptr if the task doesn't contain scan
+     * @param range the range information, nullptr if the task doesn't contain range


if the task doesn't contain range の最後の range は scan のままでいいです。タスクが scan処理を含まなければscan_infoやrangeの指定は不要だという意味なので。

kuron99 · 2024-11-12T00:53:43Z

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

-{
-    (void)resource_;  //TODO remove if not necessary
-}
+operator_builder::operator_builder(std::shared_ptr<processor_info> info,


このファイル全体にフォーマッターが適用されているように見えますが、意図的なものでしょうか？

不要なフォーマット変更がdiffに含まれているようなので、レビューしやすいように今回の変更に必要なものだけにしてもらえますか。

もしフォーマットを一括で変更したい場合は別途相談してください。
またその場合もレビューの邪魔にならないようにコミットを分けて適用しましょう。

kuron99 · 2024-11-12T00:55:25Z

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

@@ -83,14 +83,14 @@ class operator_builder {
     * @param compiler_ctx compiler context
     * @param io_info I/O information
     * @param relation_io_map mapping from relation to I/O index
-     * @param resource the memory resource used to building operators
+     * @param request_context the memory resource used to building operators


the memory resource used to building operators の部分も合わせて変更してください。

kuron99 · 2024-11-12T02:34:02Z

src/jogasaki/executor/process/abstract/range.h

 */
-class scan_info {
-public:
+class range {


趣味の問題に近いですが、rangeという単純名だとstd::rangeみたいなものもあるので、scan_rangeあたりにしませんか？rangeだと一般名で検索でヒットしすぎるので少しだけ修飾したいという思いです。

kuron99 · 2024-11-12T02:37:07Z

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

+    range& operator=(range const& other)     = delete;
+    range(range&& other) noexcept            = default;
+    range& operator=(range&& other) noexcept = default;
+    [[nodiscard]] bound const* begin() const noexcept;


rangeが保持するboundがnullptrであるという状態はどういうレンジを表現していると考えるべきでしょうか？もしその状態が必須でなければ、レンジは必ずboundを持つことにして、この関数の戻り値を bound const& にしてもいいような気がします。その方が呼び出し側で boundがnullptrがどうかというチェックが不要になるので使いやすそう。

kuron99 · 2024-11-12T05:30:25Z

src/jogasaki/kvs/storage.h

@@ -238,6 +237,7 @@ inline std::ostream& operator<<(std::ostream& out, storage const& value) {
    out << "storage(handle:" << std::hex << value.handle() << ")";
    return out;
 }
-
+end_point_kind adjust_endpoint_kind(bool use_secondary, kvs::end_point_kind endpoint);


doc commentを追加して用途と目的を簡単に書いてください。

kuron99

レビューしてコメントしました。

YoshiakiNishimura · 2024-11-13T05:19:58Z

@kuron99 指摘反映しました

kuron99 · 2024-11-13T05:55:25Z

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

すごい細かい話ですが、ファイルの末尾に改行だけを含む空の行を追加してもらえますか？
古いC言語の規約でファイル末尾は空の行でないとならないという話があって、最近のC++では関係ないはずなのですが、各種フォーマッタがこれに準じて勝手に空行を挿入しgitにdiffとして検出されてしまうことがあるので。

kuron99 · 2024-11-13T06:00:50Z

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

+    : begin_(std::move(begin)), end_(std::move(end)), is_empty_(is_empty) {
+
+    BOOST_ASSERT(&begin_);
+    BOOST_ASSERT(&end_);


この2つのassertはthisがnullptrになる場合くらいしかfalseにならないのであまり意味がなさそうに見えます。

kuron99 · 2024-11-13T06:06:42Z

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

@@ -20,18 +20,17 @@

 namespace jogasaki::executor::process::impl {

-class range : public abstract::range {
+class scan_range : public abstract::range {


scan_rangeをコンテナに入れることがありそうな気がするので、デフォルトコンストラクタも追加しませんか。空のレンジ(is_empty_ = true)を作るものということにするといいと思います。

kuron99 · 2024-11-13T06:07:41Z

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

クラス名に合わせてファイル名も scan_range.h/.cppへ変更お願いします。

kuron99 · 2024-11-13T06:08:41Z

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

こちらのファイルも末尾に空行追加おねがいします。

kuron99 · 2024-11-13T06:11:01Z

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

@@ -39,14 +39,14 @@ class operator_container {
     * @param root the root of the operator tree
     * @param operator_count the number of operators
     * @param io_exchange_map the mapping from input/output index to exchange
-     * @param range the range gathered from the scan operator in the operator tree (if any).
+     * @param scan_range the range gathered from the scan operator in the operator tree (if any).


@param の直後は変数名なのでここは range のままでOKです。

kuron99 · 2024-11-13T06:12:40Z

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

@@ -53,7 +53,7 @@ class scan_context : public context_base {
        std::unique_ptr<kvs::storage> stg,
        std::unique_ptr<kvs::storage> secondary_stg,
        transaction_context* tx,
-        impl::range const* range_,
+        impl::scan_range const* range_,


仮変数名は末尾のアンダースコア不要ですね。

kuron99 · 2024-11-13T06:35:29Z

mock/jogasaki/executor/process/mock/task_context.cpp

-
-}
-
+} // jogasaki::executor::process::mock


ここも空行を維持でお願いします。

kuron99 · 2024-11-13T06:36:17Z

mock/jogasaki/executor/process/mock/task_context.h

 };
-
-}
+} // namespace jogasaki::executor::process::mock


ここも空行を維持でお願いします。

kuron99 · 2024-11-13T06:36:32Z

src/jogasaki/data/aligned_buffer.cpp

+
+}
+
+} // namespace jogasaki::data


ここも空行を維持でお願いします。

kuron99

レビュー完了しました。ファイル末尾の空行は全部指摘していませんが、抜けているものがあれば空行を維持する方向でお願いします。

kuron99 · 2024-11-13T09:05:03Z

@YoshiakiNishimura #129 (comment) だけまだ未対応でしょうか？

kuron99

上記( #129 (comment) )が完了したらマージしてもらってOKです。

YoshiakiNishimura force-pushed the move-key-to-scan-info4 branch 3 times, most recently from f3cc94f to d304948 Compare October 31, 2024 07:50

YoshiakiNishimura force-pushed the move-key-to-scan-info4 branch from d304948 to cb49132 Compare October 31, 2024 10:28

YoshiakiNishimura force-pushed the move-key-to-scan-info4 branch 2 times, most recently from 286bf39 to c492896 Compare November 11, 2024 10:05

Replaced scan_info definition with the range class

5323388

YoshiakiNishimura force-pushed the move-key-to-scan-info4 branch from c492896 to 5323388 Compare November 11, 2024 10:19

YoshiakiNishimura marked this pull request as ready for review November 11, 2024 10:40

YoshiakiNishimura requested a review from kuron99 as a code owner November 11, 2024 10:40

Merge branch 'master' into move-key-to-scan-info4

e8c2287

kuron99 reviewed Nov 12, 2024

View reviewed changes

kuron99 requested changes Nov 12, 2024

View reviewed changes

YoshiakiNishimura marked this pull request as draft November 13, 2024 04:50

refactor and rename range to scan_range

75b8abf

YoshiakiNishimura force-pushed the move-key-to-scan-info4 branch from 7d71a44 to 75b8abf Compare November 13, 2024 04:56

YoshiakiNishimura marked this pull request as ready for review November 13, 2024 05:19

kuron99 reviewed Nov 13, 2024

View reviewed changes

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

Copy link

Contributor

kuron99 Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

こちらのファイルも末尾に空行追加おねがいします。

kuron99 reviewed Nov 13, 2024

View reviewed changes

src/jogasaki/data/aligned_buffer.cpp Outdated

}

} // namespace jogasaki::data

Copy link

Contributor

kuron99 Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここも空行を維持でお願いします。

kuron99 requested changes Nov 13, 2024

View reviewed changes

YoshiakiNishimura marked this pull request as draft November 13, 2024 07:14

YoshiakiNishimura force-pushed the move-key-to-scan-info4 branch 2 times, most recently from 726575d to 0814847 Compare November 13, 2024 07:52

YoshiakiNishimura marked this pull request as ready for review November 13, 2024 08:22

kuron99 approved these changes Nov 13, 2024

View reviewed changes

rafactor and rename range file to scan_range

bb0ff5e

YoshiakiNishimura force-pushed the move-key-to-scan-info4 branch from 0814847 to bb0ff5e Compare November 13, 2024 09:13

YoshiakiNishimura merged commit 4d467d6 into master Nov 13, 2024
10 checks passed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replaced scan_info definition with the range class #129

Replaced scan_info definition with the range class #129

YoshiakiNishimura commented Oct 31, 2024

YoshiakiNishimura commented Oct 31, 2024

YoshiakiNishimura commented Oct 31, 2024 •

edited

Loading

kuron99 commented Nov 11, 2024

YoshiakiNishimura commented Nov 11, 2024

kuron99 commented Nov 11, 2024

kuron99 commented Nov 11, 2024

YoshiakiNishimura commented Nov 11, 2024

kuron99 commented Nov 11, 2024

YoshiakiNishimura commented Nov 11, 2024

kuron99 Nov 12, 2024

kuron99 Nov 12, 2024

kuron99 Nov 12, 2024

kuron99 Nov 12, 2024

kuron99 Nov 12, 2024

kuron99 Nov 12, 2024

kuron99 Nov 12, 2024

kuron99 left a comment

YoshiakiNishimura commented Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024 •

edited

Loading

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 Nov 13, 2024

kuron99 left a comment

kuron99 commented Nov 13, 2024

kuron99 left a comment

Replaced scan_info definition with the range class #129

Replaced scan_info definition with the range class #129

Conversation

YoshiakiNishimura commented Oct 31, 2024

YoshiakiNishimura commented Oct 31, 2024

YoshiakiNishimura commented Oct 31, 2024 • edited Loading

テスト失敗メモ

場所

テスト考察

kuron99 commented Nov 11, 2024

YoshiakiNishimura commented Nov 11, 2024

kuron99 commented Nov 11, 2024

kuron99 commented Nov 11, 2024

YoshiakiNishimura commented Nov 11, 2024

kuron99 commented Nov 11, 2024

YoshiakiNishimura commented Nov 11, 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

Choose a reason for hiding this comment

kuron99 left a comment

Choose a reason for hiding this comment

YoshiakiNishimura commented Nov 13, 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 Nov 13, 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

kuron99 left a comment

Choose a reason for hiding this comment

kuron99 commented Nov 13, 2024

kuron99 left a comment

Choose a reason for hiding this comment

YoshiakiNishimura commented Oct 31, 2024 •

edited

Loading

kuron99 Nov 13, 2024 •

edited

Loading