near search: add `min_interval` #1520

HashidaTKS · 2023-02-10T02:57:13Z

Add an argument to specify min_interval for the near search family.
The intervals of phrases(words) should be greater or equals to min_interval.

The default value of min_interval is INT32_MIN(-2147483648).

New syntax:

*N${MAX_INTERVAL},${MAX_TOKEN_INTERVAL_1}|${MAX_TOKEN_INTERVAL_2}|...,${MIN_INTERVAL} "word1 word2 ..."
*NP${MAX_INTERVAL},${ADDITIONAL_LAST_INTERVAL},${MAX_PHRASE_INTERVAL_1}|${MAX_PHRASE_INTERVAL_2}|...,${MIN_INTERVAL} "phrase1 phrase2 ..."
*NPP${MAX_INTERVAL},${ADDITIONAL_LAST_INTERVAL},${MAX_PHRASE_INTERVAL_1}|${MAX_PHRASE_INTERVAL_2}|...,${MIN_INTERVAL} "(phrase1-1 phrase1-2 ...) (phrase2-1 phrase2-2 ...) ..."
*ONP${MAX_INTERVAL},${ADDITIONAL_LAST_INTERVAL},${MAX_PHRASE_INTERVAL_1}|${MAX_PHRASE_INTERVAL_2}|...,${MIN_INTERVAL} "phrase1 phrase2 ..."
*ONPP${MAX_INTERVAL},${ADDITIONAL_LAST_INTERVAL},${MAX_PHRASE_INTERVAL_1}|${MAX_PHRASE_INTERVAL_2}|...,${MIN_INTERVAL} "(phrase1-1 phrase1-2 ...) (phrase2-1 phrase2-2 ...) ..."

We should also specify ${MAX_INTERVAL}, ${ADDITIONAL_LAST_INTERVAL}, ${MAX_PHRASE_INTERVAL_1}( exclude *N) in order to specify ${MIN_INTERVAL} because it is added as the 4th argument.

Example:


table_create Entries TABLE_NO_KEY
[[0,0.0,0.0],true]
column_create Entries content COLUMN_SCALAR Text
[[0,0.0,0.0],true]
table_create Terms TABLE_PAT_KEY ShortText   --default_tokenizer 'TokenNgram("unify_alphabet", false,                                   "unify_digit", false)'   --normalizer NormalizerNFKC121
[[0,0.0,0.0],true]
column_create Terms entries_content COLUMN_INDEX|WITH_POSITION Entries content
[[0,0.0,0.0],true]
load --table Entries
[
{"content": "abc123456789def"}
]
[[0,0.0,0.0],1]
select Entries   --filter 'content *NP-1,0,,12"abc def"'   --output_columns '_score, content'
[[0,0.0,0.0],[[[0],[["_score","Int32"],["content","Text"]]]]]
select Entries   --filter 'content *NP-1,0,,11"abc def"'   --output_columns '_score, content'
[
  [
    0,
    0.0,
    0.0
  ],
  [
    [
      [
        1
      ],
      [
        [
          "_score",
          "Int32"
        ],
        [
          "content",
          "Text"
        ]
      ],
      [
        1,
        "abc123456789def"
      ]
    ]
  ]
]

In the case above, the interval between abc and def of abc123456789def is 11.
So *NP-1,0,,11 matches and *NP-1,0,,12 doesn't match.

HashidaTKS · 2023-02-13T08:19:43Z

Currently, some tests are failing.
As far as I confirmed, data.max_interval and this.max_interval are correctly the input value , -1 for example, in

groonga/lib/mrb/scripts/scan_info.rb

Line 10 in 870f426

def apply(data)

but after that, in

groonga/lib/mrb/mrb_expr.c

Line 1236 in 870f426

sis[i] = si;

, si->max_interval is always 0...

HashidaTKS · 2023-02-13T08:20:59Z

This behaviour is caused by my changes, but I can't understand what of my changes does actually affect as I didn't touch max_interval...

HashidaTKS · 2023-02-13T08:29:52Z

Hmm... I have idiotically misspelled the function name...

static mrb_value
mrb_grn_scan_info_set_min_interval(mrb_state *mrb, mrb_value self)
{
  scan_info *si;
  mrb_int min_interval;

  mrb_get_args(mrb, "i", &min_interval);
  si = DATA_PTR(self);
  grn_scan_info_set_max_interval(si, (int)min_interval); ***should be grn_scan_info_set_min_interval
  return self;
}

That case means these are a same word at that context.

skipped when GRN_OP_ORDERED_NEAR_PHRASE

HashidaTKS · 2023-02-22T06:03:31Z

@kou

Would you please review this?

HashidaTKS · 2023-02-22T06:27:25Z

...mmand/suite/select/filter/near_phrase/min_interval/with_max_element_intervals/match.expected

+{"content": "abc123456789def"}
+]
+[[0,0.0,0.0],1]
+select Entries   --filter 'content *NP-1,0,12,11"abc def"'   --output_columns '_score, content'


Currently, when max_elements_intervals specified, the interval between abc and def for abc123456789def is regarded as:

11 when checking min_interval.
12 when checking max interval specified by max_element_intervals.

This is because when checking max_interval and min_interval, the interval is calculated as 11 (

groonga/lib/ii.c

Line 12031 in b50a988

interval -= (data->token_info->n_tokens_in_phrase - 1);

)
And when checking max_element_intervals and min_interval, the interval is calculated as 12. (

groonga/lib/ii.c

Line 11766 in b50a988

int32_t interval = pos - previous_pos;

)
The diff of these intervals come from we don't subtract tokens in a phrase when calculating interval for checking max_element_intervals and min_interval.

I think this is a bug and I plan to work to fix this #1519.

lib/expr.c

kou · 2023-02-22T06:15:05Z

lib/grn_ecmascript.lemon

+      int min_interval;
+      GRN_INT32_POP(&efsi->min_interval_stack, min_interval);
+      grn_expr_append_const_int(efsi->ctx, efsi->e, min_interval,
+                                GRN_OP_PUSH, 1);
+      n_args++;


Is it correct?
Does this mean if and only if max_element_intervals is specified, min_interval must be specified too?

My intention is:

Users must also specify max_element_intervals in order to specify min_interval.

They can omit the actual value of max_element_intervals like *NP-1,0,,10 "hoge hoge"

Users can omit min_interval.

if this is out of if (max_element_intervals), min_interval could be the 3rd argument, but I couldn't get any good idea to decide that the 3rd argument is min_interval or max_element_intervals.

e.g. *NP-1,0,3 "hoge hoge", we can't decide that users intend 3 is min_interval or max_element_intervals.

min_interval must be specified too?

As the comment above, we can omit min_interval in the current implementation.

For example, in the current implementation, if we specify the filter or query as below, min_interval is the default value INT32_MIN.

*NP-1,0,2|1 "a 2 c"

(I confirmed that with a debugger.)

I will modify this as term of readability.
Please wait a while...

We always a value to efsi->min_interval_stack but this pop conditionally.
Does this work with --filter 'X *N "A B" && X *N,1,2,3 "A B"? Does the second *N use DEFAULT_MIN_INTERVAL not 3?

As far as I confirmed, it works fine. The second *N uses 3.
I have added tests for those cases.

lib/ii.c

lib/mrb/scripts/scan_info_data.rb

test/command/suite/select/filter/near/min_interval/with_max/match.expected

to GRN_II_DEFAULT_NEAR_MIN_INTERVAL

HashidaTKS · 2023-02-27T01:10:49Z

@kou

Would you re-review this?

HashidaTKS · 2023-02-27T03:01:51Z

Hmm, I noticed that we have no test case for using this with ADDITIONAL_LAST_INTERVAL.
I will add them.

HashidaTKS · 2023-02-27T05:59:18Z

Hmm, I noticed that we have no test case for using this with ADDITIONAL_LAST_INTERVAL.
I will add them.

Added.

kou · 2023-02-27T06:46:13Z

We always a value to efsi->min_interval_stack but this pop conditionally.
Does this work with --filter 'X *N "A B" && X *N,1,2,3 "A B"? Does the second *N use DEFAULT_MIN_INTERVAL not 3?

As far as I confirmed, it works fine. The second *N uses 3. I am adding tests for those cases...

私もgdbで確認しましたが、値は正しいものを使いますがefsi->min_interval_stackにはINT32_MINが残りました。

POPは必ずしませんか？

diff --git a/lib/grn_ecmascript.lemon b/lib/grn_ecmascript.lemon
index 33f2feccd..e8baadacc 100644
--- a/lib/grn_ecmascript.lemon
+++ b/lib/grn_ecmascript.lemon
@@ -399,6 +399,8 @@ relational_expression ::= relational_expression NEAR shift_expression. {
     grn_obj *max_element_intervals;
     GRN_PTR_POP(&efsi->max_element_intervals_stack,
                 max_element_intervals);
+    int min_interval;
+    GRN_INT32_POP(&efsi->min_interval_stack, min_interval);
     if (!max_element_intervals) {
       break;
     }
@@ -406,8 +408,6 @@ relational_expression ::= relational_expression NEAR shift_expression. {
                         GRN_OP_PUSH, 1);
     grn_expr_take_obj(efsi->ctx, efsi->e, max_element_intervals);
     n_args++;
-    int min_interval;
-    GRN_INT32_POP(&efsi->min_interval_stack, min_interval);
     grn_expr_append_const_int(efsi->ctx, efsi->e, min_interval,
                               GRN_OP_PUSH, 1);
     n_args++;

kou · 2023-02-27T06:48:11Z

あと、この変更の問題ではないんですが、 grn_ecmascript.lemonのquery_element ::= RELATIVE_OP query_element.にもquery_element ::= IDENTIFIER RELATIVE_OP query_element.でやっているようなPOPがないとスタックにゴミが溜まっていってしまっていました。

Always pop `min_interval`

HashidaTKS · 2023-02-27T07:07:40Z

私もgdbで確認しましたが、値は正しいものを使いますがefsi->min_interval_stackにはINT32_MINが残りました。

POPは必ずしませんか？

なるほど、確認不足でした。。。
ご指摘の通り修正し、手元で近傍検索系の全てのテストが通ることを確認しました。

HashidaTKS · 2023-02-27T07:09:57Z

あと、この変更の問題ではないんですが、 grn_ecmascript.lemonのquery_element ::= RELATIVE_OP query_element.にもquery_element ::= IDENTIFIER RELATIVE_OP query_element.でやっているようなPOPがないとスタックにゴミが溜まっていってしまっていました。

こちらについては、issue #1528 に起票しましたので、別のタスクとして実施させてください。

kou · 2023-02-27T06:45:06Z

lib/grn_ecmascript.lemon

+    grn_expr_append_const_int(efsi->ctx, efsi->e, min_interval,
+                              GRN_OP_PUSH, 1);
+    n_args++;
+  } while(0);


Suggested change

} while(0);

} while (false);

修正しました。

kou · 2023-02-27T06:49:47Z

lib/grn_ii.h

@@ -200,6 +202,7 @@ struct _grn_select_optarg {
  int additional_last_interval;
  grn_obj *query_options;
  grn_obj *max_element_intervals;
+  int *min_interval;


grn_select_optargは公開インターフェイスじゃないのでintでいい気がします。

intに直しました。

kou · 2023-02-27T06:53:19Z

lib/ii.c

+    return (interval <=
+             (data->max_interval + data->additional_last_interval) &&
+            interval_without_last_token <= data->max_interval);


私はカッコが残っていたほうが好みですが、カッコを取りたいなら中途半端に取らないでもっと取りませんか？

Suggested change

return (interval <=

(data->max_interval + data->additional_last_interval) &&

interval_without_last_token <= data->max_interval);

return interval <= (data->max_interval + data->additional_last_interval) &&

interval_without_last_token <= data->max_interval;

ここにmin_intervalを入れていたときに追加した変更で、現在は不要な変更なので、ここの変更自体をリバーとしました。

kou · 2023-02-27T06:56:40Z

lib/proc.c

@@ -4350,6 +4350,7 @@ selector_in_values(grn_ctx *ctx, grn_obj *table, grn_obj *index,
    search_options.similarity_threshold = 0;
    search_options.max_interval = 0;
    search_options.additional_last_interval = 0;
+    search_options.min_interval = NULL;


grn_search_optargでのメンバーの定義順と同じ順番にしませんか？

修正しました。

lib/table_selector.c

kou · 2023-02-27T07:09:08Z

...nd/suite/select/filter/near_phrase/min_interval/with_additional_last_interval/match.expected

+{"content": "abc123456789def"}
+]
+[[0,0.0,0.0],1]
+select Entries   --filter 'content *NP12,1,,10"abc def$"'   --output_columns '_score, content'


10じゃなくて11がボーダーじゃないですか？

おっしゃるとおりですので、11に直しました。
（色々試しているときに10を入れてそのままになっていました）

Co-authored-by: Sutou Kouhei <kou@clear-code.com>

HashidaTKS · 2023-02-27T07:49:20Z

@kou

度々のご確認ありがとうございます。ご指摘の事項に対応しました。

HashidaTKS added 20 commits February 22, 2023 14:38

Add min_interval for syntaxx

7e21e25

Fix parsing bug

b4b9a08

Assign select_optarg->min_interval

100b970

Modify scan_info_data.rb for adding min_interval

f920f36

Add min_interval initialization in selector_in_values

9e975dc

Fix missing implementation

b33d4fb

Fix the place of adding min_interval

ac642aa

Fix a typo

0409d1a

Add min_interval logic

033aa9c

Add min_interval logic

50996d6

Modify default of min_interval

cc418d5

Fix if condition for comparison

d46a7b1

Add tests

481ebcc

Add min_interval check

5e0ee74

*ONP: skip if "interval == 0"

a51e423

That case means these are a same word at that context.

Add tests for query

50edc47

Add tests for filter

42162ca

Remove temporary changes

63ba3ec

Fix a bug that some max_element_interval check is

7abbe06

skipped when GRN_OP_ORDERED_NEAR_PHRASE

Fix a typo (overrap -> overlap)

57fccbe

HashidaTKS force-pushed the add-min-interval branch from 94a3b0b to 57fccbe Compare February 22, 2023 05:46

Rename tests path

b50a988

HashidaTKS changed the title ~~[WIP] near search: add min_interval~~ near search: add min_interval Feb 22, 2023

HashidaTKS marked this pull request as ready for review February 22, 2023 06:07

HashidaTKS commented Feb 22, 2023

View reviewed changes

kou reviewed Feb 22, 2023

View reviewed changes

HashidaTKS added 4 commits February 24, 2023 17:17

Keep backward commpatiblity

9125cbf

Assign correct value

39421f6

Align function position

0458ed6

Rename GRN_SELECT_DEFAULT_NEAR_MIN_INTERVAL

14d5f8e

to GRN_II_DEFAULT_NEAR_MIN_INTERVAL

HashidaTKS mentioned this pull request Feb 27, 2023

NormalizerNFKC: add unify_kana_hyphen option #1526

Merged

Add tests for using with additional_last_interval

c45b988

Apply the feedback

6989d4e

Always pop `min_interval`

HashidaTKS mentioned this pull request Feb 27, 2023

query_element ::= RELATIVE_OP query_element. でスタックにゴミが残らないようにする #1528

Open

kou reviewed Feb 27, 2023

View reviewed changes

HashidaTKS and others added 7 commits February 27, 2023 16:19

Revert needless change

b1f253c

Modify while(0) to while(false)

408c90b

Make min_interval in _grn_select_optarg int

9c3f95c

Update lib/table_selector.c

1f0fb82

Co-authored-by: Sutou Kouhei <kou@clear-code.com>

Align search_options.min_interval

be41036

parentheses

a0cf08c

Fix matching test cases

68d90c3

kou added 3 commits February 27, 2023 16:52

Add a missing space between while and (false)

0acb91c

Revert needless style change

c60b464

Use the same order

ce669e8

kou merged commit c6107af into master Feb 27, 2023

kou deleted the add-min-interval branch February 27, 2023 07:57

github-actions bot mentioned this pull request May 22, 2023

nginx: update bundles version 1.23.4 komainu8/groonga#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

near search: add `min_interval` #1520

near search: add `min_interval` #1520

HashidaTKS commented Feb 10, 2023 •

edited

HashidaTKS commented Feb 13, 2023

HashidaTKS commented Feb 13, 2023 •

edited

HashidaTKS commented Feb 13, 2023 •

edited

HashidaTKS commented Feb 22, 2023

HashidaTKS Feb 22, 2023 •

edited

kou Feb 22, 2023

HashidaTKS Feb 22, 2023 •

edited

HashidaTKS Feb 24, 2023 •

edited

HashidaTKS Feb 24, 2023

HashidaTKS Feb 24, 2023

kou Feb 24, 2023

HashidaTKS Feb 27, 2023

HashidaTKS commented Feb 27, 2023

HashidaTKS commented Feb 27, 2023

HashidaTKS commented Feb 27, 2023

kou commented Feb 27, 2023

kou commented Feb 27, 2023

HashidaTKS commented Feb 27, 2023

HashidaTKS commented Feb 27, 2023

kou Feb 27, 2023

HashidaTKS Feb 27, 2023

kou Feb 27, 2023

HashidaTKS Feb 27, 2023

kou Feb 27, 2023

HashidaTKS Feb 27, 2023

kou Feb 27, 2023

HashidaTKS Feb 27, 2023

kou Feb 27, 2023

HashidaTKS Feb 27, 2023

HashidaTKS commented Feb 27, 2023

near search: add min_interval #1520

near search: add min_interval #1520

Conversation

HashidaTKS commented Feb 10, 2023 • edited

HashidaTKS commented Feb 13, 2023

HashidaTKS commented Feb 13, 2023 • edited

HashidaTKS commented Feb 13, 2023 • edited

HashidaTKS commented Feb 22, 2023

HashidaTKS Feb 22, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HashidaTKS Feb 22, 2023 • edited

Choose a reason for hiding this comment

HashidaTKS Feb 24, 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

HashidaTKS commented Feb 27, 2023

HashidaTKS commented Feb 27, 2023

HashidaTKS commented Feb 27, 2023

kou commented Feb 27, 2023

kou commented Feb 27, 2023

HashidaTKS commented Feb 27, 2023

HashidaTKS commented Feb 27, 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

HashidaTKS commented Feb 27, 2023

near search: add `min_interval` #1520

near search: add `min_interval` #1520

HashidaTKS commented Feb 10, 2023 •

edited

HashidaTKS commented Feb 13, 2023 •

edited

HashidaTKS commented Feb 13, 2023 •

edited

HashidaTKS Feb 22, 2023 •

edited

HashidaTKS Feb 22, 2023 •

edited

HashidaTKS Feb 24, 2023 •

edited