Feature #1934

Rubyでクエリーオプティマイザーを書けるようにする

Added by Kouhei Sutou almost 5 years ago. Updated over 4 years ago.

Status:新規Start date:09/06/2013
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

動機

現在のgroongaはクエリーオプティマイザーをCで書いている。Cで書いているため、こんな最適化はどうだろうか?などといったことを簡単に試すことができない。最適化することで1桁2桁速度が速くなることもあるので、いろいろな最適化を簡単に試すことができる仕組みが欲しい。

ゴール

Rubyでクエリーオプティマイザーを書ける。lib/expr.c:scan_info_build()の中身をRubyで書けるとよい。

クエリーオプティマイザーでは、こんな最適化ができるとよい。(これはこのチケットのゴールの範囲外。このチケットのゴールができた後の話。)

現在は"10 < a && a < 20"というクエリーを以下のように実行している。

  • "10 < a"のレコードをインデックスで範囲検索してすべて取得する
  • "a < 20"のレコードをインデックスで範囲検索してすべて取得する
  • どちらにも含まれているレコードだけを結果として残す

しかし、上限と下限を指定して範囲検索することもできるため、最適化して以下のように実行したい。

  • "10 < a < 20"のレコードをインデックスで範囲検索してすべて取得する

実現案

mrubyをgroongaに組み込み、Rubyでクエリーオプティマイザーを書けるようにする。mrubyの方がCより遅いがクエリーオプティマイザーの処理は検索処理に比べれば軽いので速度は問題にならないはずである。むしろ、クエリーを最適化することにより現在よりも速くなる可能性がある。

現在の実装は、grn_ctx毎にmrubyのインスタンスを持てるところまでいっている。(grn_ctx.impl.mrbがmruby_state) しかし、groongaのmrubyバインディングがないため、mrubyからgroongaの情報を取得できないし、mrubyでgroongaのオブジェクトも作れない。

この状態からlib/expr.c:scan_info_build()の中身をRubyで書けるようになるところまでいきたい。"10 < a && a < 20"の最適化などは次の段階だろう。

mrubyバインディングの作成は以下の方針でいきたい。

  • mrubyからはgroongaオブジェクトのメモリ管理をしない。単に参照するだけ。理由はGCによる停止期間を小さくしたいのと、クラッシュが怖いから。

    • メモリプールみたいなCのオブジェクトを作ってそいつがメモリ管理するのがいいんじゃないかと思っている。aprのapr_poolとかnginxのngx_pool_tとかObjective-CのNSAutoreleasePoolみたいなイメージ。どうせ、scan_info_build()の中でしかRubyのコードを動かさないことがわかっているので、その間で作られたものはRubyのコードが終わったら開放できる。コードにするとこんなイメージ。

      scan_info_build()
      {
        memory_pool *pool;
        pool = pool_new();
        mruby_plugin_exec(pool)
        pool_destroy(pool);
      }
      
  • 最初にmruby_execコマンドを追加するプラグインを作ったほうがいいかもしれない。テストしやすそうだし。

History

#1 Updated by Kouhei Sutou almost 5 years ago

  • Description updated (diff)

#2 Updated by _ wanabe over 4 years ago

大変遅くなりました。 pull request を数個送りましたので見ていただけるとありがたいです。

https://github.com/groonga/groonga/pull/105

https://github.com/groonga/groonga/pull/106

https://github.com/groonga/groonga/pull/107

https://github.com/groonga/groonga/pull/108

上三つが独立したパッチで、最後のものがそれらを統合した上で実際にスクリプトにしたものです。

#3 Updated by Kouhei Sutou over 4 years ago

ありがとうございます!

pull requestの扱いについてまだピンときていないので教えてください! 108には105-107が全部入っているんですよね?これは、取り込むときは108を取り込んでね、105-107は参考までに、みたいなイメージですか? (108が取り込まれたら105-107はreject扱いにする。) それとも、105-107を最初に取り込んでね、108はそれらの後にしてね、そしたらgitがいい感じにマージしてくれるはずだから、みたいなイメージですか? (108を105-107の前にマージしてはダメ!)

同じ内容のコミットが入ったpull requestが複数あるケースは初めてだったのでどういう流れを想定しているか教えて欲しいです!

#4 Updated by Kouhei Sutou over 4 years ago

内容に関してですが、大きな方向性はよいと思うのですが、これをそのまま取り込むのは難しいので少しずつ調整しながら取り込んでいければいいなぁという感じです。

どれから入れていくのがいいですかねぇ。

mrbcは後回しでいいかなぁと思います。せっかくオルbyで書けるようになったのにビルドしなおさないとオプティマイザーを試せないのは切ないので。。。オプションとしてmrbcで.cにすれば速くなるというのがあればいいと思います。ただ、.cにしたら本当に速くなるか、速くなるとしたらどのくらい速くなるか、みたいなのは最終的に導入する前には計測したいなぁと思います。

バインディングがmrb.cに散らばっているのはファイルを分割したい感じです。lib/mrb/を作ったほうがよさそうですね。それは私が作っておきますね。

最初に説明に書き忘れていたのですが、既存のクエリーオプティマイザーを置き換える前にmrubyでのクエリーオプティマイザーで割にあうか性能を検証してから最終的に判断したいと思っています。そのため、expr.cの既存のコードはできるだけ変更したくないなぁと思っています。その変更が影響して「現時点の」クエリーオプティマイザーの性能が落ちてしまう可能性もあるからです。

今の段階では、既存のコードを変更しなければいけない部分(例えば、grn_scan_info_alloc()を作っているところとか)は、コピペしてしまっていいです。最終的にmrubyでいくぞ、っとなったら既存のコードを捨ててしまえばいいので。

という感じのことを思いました!

どこから着手するのがやりやすいですか?

#5 Updated by _ wanabe over 4 years ago

> 同じ内容のコミットが入ったpull requestが複数あるケースは初めてだったので

変な request の仕方をしてしまってすみません。105-107 がないと 108 は意味を成さないので…… 自分としては、

> 105-107を最初に取り込んでね、108はそれらの後にしてね

のつもりでした。 もしマージされなかったり不都合があれば、108 は一旦閉じて改めて pull request をさせて頂きます。

> 少しずつ調整しながら取り込んでいければいいなぁという感じです。

すみません、お手数かけます……

> どこから着手するのがやりやすいですか?

とりあえず mrbc を使わないようにするのと、expr.c の既存コードを変えないようにするところから やってみようと思います。

それで質問なのですが、スクリプトファイルはどこに配置するのが適切なのでしょうか? etc/groonga(sysconfdir)か share/groonga か、はたまたそれ以外かで悩んでいます。 おそらく sysconfdir だろう、と思うのですが。

また groonga.conf のように、デフォルトパスを config.h に埋め込んでおいて 環境変数で指定された場合は環境変数のパス、そうでなければデフォルトのパスを見る、というように するのを想定していたのですが、それで問題ないでしょうか? その場合、指定できるのは ・ディレクトリのパスのみ ・スクリプトファイルのフルパスのみ ・ディレクトリのパスとスクリプトファイル名、それぞれ別々に の三通り考えられますが、どれが望ましいでしょうか?

#6 Updated by Kouhei Sutou over 4 years ago

_ wanabe wrote:

> 同じ内容のコミットが入ったpull requestが複数あるケースは初めてだったので


変な request の仕方をしてしまってすみません。105-107 がないと 108 は意味を成さないので……

いえいえ!

自分としては、


> 105-107を最初に取り込んでね、108はそれらの後にしてね


のつもりでした。
もしマージされなかったり不都合があれば、108 は一旦閉じて改めて pull request をさせて頂きます。

なるほど!わかりましたー 説明ありがとうございます!

> 少しずつ調整しながら取り込んでいければいいなぁという感じです。


すみません、お手数かけます……


> どこから着手するのがやりやすいですか?


とりあえず mrbc を使わないようにするのと、expr.c の既存コードを変えないようにするところから
やってみようと思います。

わかりました。pull requestは別々になっていた方が取り込みやすいです。

できればテストも追加していきたいのですが、どうするのがいいでしょうねぇ。 ruby_evalコマンドの場合はtest/command/suite/以下でテストできたんですが、今触っているところはもっと中のところなのでそれはできないですし。。。Cutterで書くとCなのでつらそうだし。。。うーん。

それで質問なのですが、スクリプトファイルはどこに配置するのが適切なのでしょうか?
etc/groonga(sysconfdir)か share/groonga か、はたまたそれ以外かで悩んでいます。
おそらく sysconfdir だろう、と思うのですが。

lib/groonga/#{何か}、がよい気がします!ライブラリーなので。 ちなみに、今はlib/groonga/plugins/にプラグインを置いています。

また groonga.conf のように、デフォルトパスを config.h に埋め込んでおいて
環境変数で指定された場合は環境変数のパス、そうでなければデフォルトのパスを見る、というように
するのを想定していたのですが、それで問題ないでしょうか?

はい、最高です!

その場合、指定できるのは
・ディレクトリのパスのみ
・スクリプトファイルのフルパスのみ
・ディレクトリのパスとスクリプトファイル名、それぞれ別々に
の三通り考えられますが、どれが望ましいでしょうか?

うーん、最初のふたつのどちらかですかねぃ。3番目はわけたときのメリットがパッとうかばなかったので。 一番上が拡張性が高い気がしますが、今はそこまで必要ない気がするのでまずは2番目がいいんじゃないかと思います。必要になったら1番目で、みたいな。

#7 Updated by _ wanabe over 4 years ago

ありがとうございます。ではコメントに従って書き直してみます。

何度もすみません、追加で質問なのですが

  • mrbc を使わないということで 108 は必然的に取り下げることになるのですが、 新しい pull request はどのようにするのがよいでしょうか?

    • 108 と同じように前提となる request 群をマージした状態で request するか、
    • 単純に私のレポジトリにだけ置いて request をしないでおくか、
    • 単体では動かない状態で構わず pull request する(前提の request 群を入れた後だと動く)か、
    • それともそれ以外の方法か、どのようにしましょう。
  • expr.c で定義されていて外部でも参照したい定数・列挙型は expr.h に出したいのですがどうしましょう? とりあえず元の状態を最大限残すために下記 1 番の方法でやってみる予定でいます。問題ないでしょうか?

    1. expr.c から #include "expr.h" するのをやめて、両方に定義が残っている状態にする
      • メリット:expr.c を元の状態に最大限近い状態にできる
      • デメリット:不整合の可能性あり。expr.h を使うとなったときの変更箇所がわかりにくい
    2. expr.c 側の定数・列挙型の定義部分を #ifndef GRN_EXPR_H 〜 #endinf などで囲む
      • メリット:実際にこの方向で行くとなったとき、変更するべき箇所がわかりやすい
      • デメリット:不整合の可能性あり。ぱっと見たときに意味がわかりにくい(コメント必須?)
    3. 現在の request のように、ヘッダに定数定義を移動させる
      • メリット:自然。不整合が起こらない
      • デメリット:棄却となったときに戻す箇所がわかりにくい

#8 Updated by Kouhei Sutou over 4 years ago

_ wanabe wrote:

ありがとうございます。ではコメントに従って書き直してみます。

ありがとうございます!

何度もすみません、追加で質問なのですが


  • mrbc を使わないということで 108 は必然的に取り下げることになるのですが、
    新しい pull request はどのようにするのがよいでしょうか?
    • 108 と同じように前提となる request 群をマージした状態で request するか、
    • 単純に私のレポジトリにだけ置いて request をしないでおくか、
    • 単体では動かない状態で構わず pull request する(前提の request 群を入れた後だと動く)か、
    • それともそれ以外の方法か、どのようにしましょう。

「単体では動かない状態」というのは「ビルドも通らない」ではなくて、「ビルドは通るし、既存の機能もそのまま動くけどmrubyでクエリーオプティマイザーを書ける状態ではない」ということですよね? であれば、「単体では動かない状態で構わず pull request する(前提の request 群を入れた後だと動く)」がよいです!

(欲をいえば、mrubyでクエリーオプティマイザーを書けない状態でも、そのpull requestで追加する機能が期待通り動くことは確認できるとうれしいです。例えば、テストがある状態です。が、私も難しいなぁと思うのでこれは欲です。)

  • expr.c で定義されていて外部でも参照したい定数・列挙型は expr.h に出したいのですがどうしましょう?
    とりあえず元の状態を最大限残すために下記 1 番の方法でやってみる予定でいます。問題ないでしょうか?


    1. expr.c から #include "expr.h" するのをやめて、両方に定義が残っている状態にする
      • メリット:expr.c を元の状態に最大限近い状態にできる
      • デメリット:不整合の可能性あり。expr.h を使うとなったときの変更箇所がわかりにくい
    2. expr.c 側の定数・列挙型の定義部分を #ifndef GRN_EXPR_H 〜 #endinf などで囲む
      • メリット:実際にこの方向で行くとなったとき、変更するべき箇所がわかりやすい
      • デメリット:不整合の可能性あり。ぱっと見たときに意味がわかりにくい(コメント必須?)
    3. 現在の request のように、ヘッダに定数定義を移動させる
      • メリット:自然。不整合が起こらない
      • デメリット:棄却となったときに戻す箇所がわかりにくい

あ、それは、3.で大丈夫です!

#9 Updated by _ wanabe over 4 years ago

Kouhei Sutou wrote:

_ wanabe wrote:

  • mrbc を使わないということで 108 は必然的に取り下げることになるのですが、
    新しい pull request はどのようにするのがよいでしょうか?
    • 108 と同じように前提となる request 群をマージした状態で request するか、
    • 単純に私のレポジトリにだけ置いて request をしないでおくか、
    • 単体では動かない状態で構わず pull request する(前提の request 群を入れた後だと動く)か、
    • それともそれ以外の方法か、どのようにしましょう。

「単体では動かない状態」というのは「ビルドも通らない」ではなくて、「ビルドは通るし、既存の機能もそのまま動くけどmrubyでクエリーオプティマイザーを書ける状態ではない」ということですよね?
であれば、「単体では動かない状態で構わず pull request する(前提の request 群を入れた後だと動く)」がよいです!

うーん、grn_mrb_send 等の追加した API にリンクしていますので --enable-mruby では(GRN_WITH_MRUBY では)ビルドは通らなくなってしまうと思います。 もちろん --enable-mruby しなければ何も問題なく動くと思います。 あ、今思いつきましたが、デフォルトでは定義されないような定数を用いて #ifdef 〜 #endif で 囲んでやればいいかもしれません。 暫定的に --enable-query-optimizer-with-mruby を追加するとか。

(欲をいえば、mrubyでクエリーオプティマイザーを書けない状態でも、そのpull requestで追加する機能が期待通り動くことは確認できるとうれしいです。例えば、テストがある状態です。が、私も難しいなぁと思うのでこれは欲です。)

そうですね。。。 そういえば、メソッドの単体テストを書き忘れていました。 複合的なテストは既存の test/command 等のテストでよいと思うのですが。

  • expr.c で定義されていて外部でも参照したい定数・列挙型は expr.h に出したいのですがどうしましょう?
    とりあえず元の状態を最大限残すために下記 1 番の方法でやってみる予定でいます。問題ないでしょうか?


    1. expr.c から #include "expr.h" するのをやめて、両方に定義が残っている状態にする
      • メリット:expr.c を元の状態に最大限近い状態にできる
      • デメリット:不整合の可能性あり。expr.h を使うとなったときの変更箇所がわかりにくい
    2. expr.c 側の定数・列挙型の定義部分を #ifndef GRN_EXPR_H 〜 #endinf などで囲む
      • メリット:実際にこの方向で行くとなったとき、変更するべき箇所がわかりやすい
      • デメリット:不整合の可能性あり。ぱっと見たときに意味がわかりにくい(コメント必須?)
    3. 現在の request のように、ヘッダに定数定義を移動させる
      • メリット:自然。不整合が起こらない
      • デメリット:棄却となったときに戻す箇所がわかりにくい


あ、それは、3.で大丈夫です!

ではそのようにいたします。ありがとうございます。

#10 Updated by Kouhei Sutou over 4 years ago

_ wanabe wrote:

うーん、grn_mrb_send 等の追加した API にリンクしていますので
--enable-mruby では(GRN_WITH_MRUBY では)ビルドは通らなくなってしまうと思います。

それは困りますねぇ。先に依存している方を取り込むことをやった方がよい気がしました。

もちろん --enable-mruby しなければ何も問題なく動くと思います。
あ、今思いつきましたが、デフォルトでは定義されないような定数を用いて #ifdef 〜 #endif で
囲んでやればいいかもしれません。
暫定的に --enable-query-optimizer-with-mruby を追加するとか。

う、うーん、ちょっと大げさかなぁと思いました><

(欲をいえば、mrubyでクエリーオプティマイザーを書けない状態でも、そのpull requestで追加する機能が期待通り動くことは確認できるとうれしいです。例えば、テストがある状態です。が、私も難しいなぁと思うのでこれは欲です。)


そうですね。。。
そういえば、メソッドの単体テストを書き忘れていました。
複合的なテストは既存の test/command 等のテストでよいと思うのですが。

ちょっと書いてみたんですが、test/command/でもまあいけるかなぁと思いました。お手軽ですし。

書いてみた例: https://github.com/groonga/groonga/commit/fc8118af88901683e8c8a0c6a5758c2981b19960

#11 Updated by _ wanabe over 4 years ago

Kouhei Sutou wrote:

_ wanabe wrote:

うーん、grn_mrb_send 等の追加した API にリンクしていますので
--enable-mruby では(GRN_WITH_MRUBY では)ビルドは通らなくなってしまうと思います。


それは困りますねぇ。先に依存している方を取り込むことをやった方がよい気がしました。

ああすみません、では 106 の書き直しをすぐにやります。それさえすれば

もちろん --enable-mruby しなければ何も問題なく動くと思います。
あ、今思いつきましたが、デフォルトでは定義されないような定数を用いて #ifdef 〜 #endif で
囲んでやればいいかもしれません。
暫定的に --enable-query-optimizer-with-mruby を追加するとか。


う、うーん、ちょっと大げさかなぁと思いました><

のような問題もなくなるはずですので。

(欲をいえば、mrubyでクエリーオプティマイザーを書けない状態でも、そのpull requestで追加する機能が期待通り動くことは確認できるとうれしいです。例えば、テストがある状態です。が、私も難しいなぁと思うのでこれは欲です。)


そうですね。。。
そういえば、メソッドの単体テストを書き忘れていました。
複合的なテストは既存の test/command 等のテストでよいと思うのですが。


ちょっと書いてみたんですが、test/command/でもまあいけるかなぁと思いました。お手軽ですし。


書いてみた例: https://github.com/groonga/groonga/commit/fc8118af88901683e8c8a0c6a5758c2981b19960

test/command/ に単体テストを書くということでしょうか?なるほど。 ですが、追加したメソッドは mrb_grn_err を除くと構造体へのアクセスをラップしたものばかりなので オブジェクトへ渡す構造体を初期化することを考えると、test/command/ からの単体テストは難しいように思います。 ああ、Scaninfo#initialize 等を追加するなら可能かもしれません。ちょっと考えてみます。

#12 Updated by _ wanabe over 4 years ago

109 のマージありがとうございます! 英語で書ける自信がないのでこちらで失礼します。

"I don't know whether grn_scan_info_each_arg() is needed or not but I've merged it." とのことですが、grn_scan_info_get_arg(grn_ctx *ctx, scan_info *si, int i) や grn_obj **grn_scan_info_get_args(grn_ctx *ctx, scan_info *si) などの方が良かったということでしょうか? これは自分でも迷った所なので、アドバイス頂けるとうれしいです。

それと細かいところですが、先ほど確認したところ 105 が master の変更に追随しておらず そのままではマージできませんでした。すみません。どうするのが都合よろしいでしょうか? 細かい修正なので pull request しなおすことも可能ですし、他の対応でも問題ありません。

#13 Updated by Kouhei Sutou over 4 years ago

_ wanabe wrote:

109 のマージありがとうございます!

いえいえ!

"I don't know whether grn_scan_info_each_arg() is needed or not but I've merged it."
とのことですが、grn_scan_info_get_arg(grn_ctx *ctx, scan_info *si, int i) や
grn_obj **grn_scan_info_get_args(grn_ctx *ctx, scan_info *si) などの方が良かったということでしょうか?
これは自分でも迷った所なので、アドバイス頂けるとうれしいです。

実際にどういう風に使われるのを想定しているのか(どういうときに使えると嬉しいか)わからなかったので、うーんとなりました。他のはアクセサーですけど、これはちょっと違いますよね。

ということで、実際にどう使うかが私はわかっていないので、どういうのがいいかもわからない感じです。。。必要になったときに追加でもいいかなぁという気がしていました。

それと細かいところですが、先ほど確認したところ 105 が master の変更に追随しておらず
そのままではマージできませんでした。すみません。どうするのが都合よろしいでしょうか?
細かい修正なので pull request しなおすことも可能ですし、他の対応でも問題ありません。

105もどう使うのが見えないのでモヤモヤしています。。。 Exprクラスを使うようなコードも入っていますがそれを定義するコードはまだ入っていないので、まだ使えないように見えますし。。。 どういう方向に向かっているかがこのpull requestのコードだけからはあんまり伝わってこないんというか。。。どう進めるのがいいですかねぃ。

私の方でクエリーオプティマイザーをテストする仕組みを作った方がいいのかしら。考えてきます。。。

#14 Updated by _ wanabe over 4 years ago

Kouhei Sutou wrote:

_ wanabe wrote:

"I don't know whether grn_scan_info_each_arg() is needed or not but I've merged it."
とのことですが、grn_scan_info_get_arg(grn_ctx *ctx, scan_info *si, int i) や
grn_obj **grn_scan_info_get_args(grn_ctx *ctx, scan_info *si) などの方が良かったということでしょうか?
これは自分でも迷った所なので、アドバイス頂けるとうれしいです。


実際にどういう風に使われるのを想定しているのか(どういうときに使えると嬉しいか)わからなかったので、うーんとなりました。他のはアクセサーですけど、これはちょっと違いますよね。


ということで、実際にどう使うかが私はわかっていないので、どういうのがいいかもわからない感じです。。。必要になったときに追加でもいいかなぁという気がしていました。

うーんなるほど。切り分け方を間違った気がひしひしとしています。

それと細かいところですが、先ほど確認したところ 105 が master の変更に追随しておらず
そのままではマージできませんでした。すみません。どうするのが都合よろしいでしょうか?
細かい修正なので pull request しなおすことも可能ですし、他の対応でも問題ありません。


105もどう使うのが見えないのでモヤモヤしています。。。
Exprクラスを使うようなコードも入っていますがそれを定義するコードはまだ入っていないので、まだ使えないように見えますし。。。
どういう方向に向かっているかがこのpull requestのコードだけからはあんまり伝わってこないんというか。。。どう進めるのがいいですかねぃ。

これも前述の「切り分け方の間違い」のせいですね。すみません。 であれば、段々と mruby スクリプト化していく、というような切り分け方はどうでしょうか。 具体的には、scan_info_build のコードをコピペして少しずつ mruby のコードに置き換えていくことを考えています。 ええと、作業そのままで汚いのですが https://github.com/wanabe/groonga/commits/mruby-plugin-test2 のコミットのような具合です。 ただこの場合、コミット自体は分けられても pull request を分けると中途半端になってしまうので そこを懸念しています。コードの意図という点ではわかりやすくなると思いますが。。。

私の方でクエリーオプティマイザーをテストする仕組みを作った方がいいのかしら。考えてきます。。。

ちょっと判断がつかないのでお任せします。お手数おかけします。

#15 Updated by Kouhei Sutou over 4 years ago

_ wanabe wrote:

であれば、段々と mruby スクリプト化していく、というような切り分け方はどうでしょうか。
具体的には、scan_info_build のコードをコピペして少しずつ mruby のコードに置き換えていくことを考えています。

それはいいやり方のように思います!

ええと、作業そのままで汚いのですが https://github.com/wanabe/groonga/commits/mruby-plugin-test2
のコミットのような具合です。
ただこの場合、コミット自体は分けられても pull request を分けると中途半端になってしまうので
そこを懸念しています。コードの意図という点ではわかりやすくなると思いますが。。。

「中途半端」というのは「少しずつbuild_scan_info()相当の処理がmrubyで動くようになっていくけど、途中の状態ではbuild_scan_info()のフル機能は提供できず、一部の機能だけの提供になってしまう」ということですよね?「ビルドもできない」といういみではなくて。

であれば、それは問題ではないと思います!

あ、違うのか。「1個の大きいpull requestになってしまうよ」ということですね?うーん、それは、最初と同じアプローチな気がするので、この部分まではいいと思うけど、それ以降はうーんというときに途中まで取り込むというのができなくて、切ないなぁという感じです。

私の方でクエリーオプティマイザーをテストする仕組みを作った方がいいのかしら。考えてきます。。。


ちょっと判断がつかないのでお任せします。お手数おかけします。

わかりました!

#16 Updated by _ wanabe over 4 years ago

Kouhei Sutou wrote:

_ wanabe wrote:

ええと、作業そのままで汚いのですが https://github.com/wanabe/groonga/commits/mruby-plugin-test2
のコミットのような具合です。
ただこの場合、コミット自体は分けられても pull request を分けると中途半端になってしまうので
そこを懸念しています。コードの意図という点ではわかりやすくなると思いますが。。。


「中途半端」というのは「少しずつbuild_scan_info()相当の処理がmrubyで動くようになっていくけど、途中の状態ではbuild_scan_info()のフル機能は提供できず、一部の機能だけの提供になってしまう」ということですよね?「ビルドもできない」といういみではなくて。


であれば、それは問題ではないと思います!

ああ、そういう意図ではなかったのですが、それはよさそうです。

あ、違うのか。「1個の大きいpull requestになってしまうよ」ということですね?うーん、それは、最初と同じアプローチな気がするので、この部分まではいいと思うけど、それ以降はうーんというときに途中まで取り込むというのができなくて、切ないなぁという感じです。

もとの発言はこの意図でした。ですが、前述の方法がよさそうなのでそれで考えてみます。 元々の作業は関数の外側から内側に向けてスクリプト化していったのですが(という表現で伝わるでしょうか……) 内側から変えていくようにすれば細かい pull request にできそうな気がします。 ありがとうございます。

#17 Updated by Kouhei Sutou over 4 years ago

_ wanabe wrote:

Kouhei Sutou wrote:

「中途半端」というのは「少しずつbuild_scan_info()相当の処理がmrubyで動くようになっていくけど、途中の状態ではbuild_scan_info()のフル機能は提供できず、一部の機能だけの提供になってしまう」ということですよね?「ビルドもできない」といういみではなくて。


であれば、それは問題ではないと思います!


ああ、そういう意図ではなかったのですが、それはよさそうです。

では、それでいきましょう!

元々の作業は関数の外側から内側に向けてスクリプト化していったのですが(という表現で伝わるでしょうか……)
内側から変えていくようにすれば細かい pull request にできそうな気がします。

そうですね!

Also available in: Atom PDF