Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expr adjust: fix invalid weight with multiple adjusts #1554

Merged
merged 13 commits into from
Apr 14, 2023
Merged

expr adjust: fix invalid weight with multiple adjusts #1554

merged 13 commits into from
Apr 14, 2023

Conversation

komainu8
Copy link
Member

@komainu8 komainu8 commented Apr 7, 2023

GitHub: fix GH-1548

The weight value that a user specify is pushed in stack.
This value pop in PARSE().
Currently, If we specify multiple weights in search query, Groonga pop the latest weight immediately after Groonga it push in stack.

Therefore, the latest weight doesn't apply result.
As a result, the previous weight is multiple applied.

In this modification, multiple weight are applied correctly by pushing weight in stack after Groonga execute PARSE().

@kou
Copy link
Member

kou commented Apr 7, 2023

Hmm...
We should pop in .lemon because nest information should be processed by Lemon.

It seems that we just need the following change:

diff --git a/lib/expr.c b/lib/expr.c
index 29fa00cea..cd7a13e70 100644
--- a/lib/expr.c
+++ b/lib/expr.c
@@ -4431,8 +4434,8 @@ parse_query_accept_adjust(grn_ctx *ctx,
                           float weight)
 {
   if (!(q->flags & GRN_EXPR_QUERY_NO_SYNTAX_ERROR)) {
-    parse_query_push_weight(ctx, q, weight);
     PARSE(token);
+    parse_query_push_weight(ctx, q, weight);
     return;
   }

@komainu8
Copy link
Member Author

@kou Thank you for your comments.
I've fixed it and I added tests for this modification.

Copy link
Member

Choose a reason for hiding this comment

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

Could you revert this?

@kou kou changed the title Move a timing for pop of weight value expr adjust: fix invalid weight with multiple adjusts Apr 11, 2023
@kou
Copy link
Member

kou commented Apr 11, 2023

Could you add why/when this problem is happen and how to fix this problem to the pull request description?

@komainu8
Copy link
Member Author

komainu8 commented Apr 14, 2023

I updated he pull request description.

@kou kou merged commit 47cdb2c into master Apr 14, 2023
@kou kou deleted the fix-1548 branch April 14, 2023 08:30
@kou
Copy link
Member

kou commented Apr 14, 2023

FYI: My version:

GitHub: fix GH-1548

Weights are managed by stack.

Currently, if we specify multiple weights in search query ("<0.1title:@groonga <0.3content:@Full"), Groonga doesn't pops the current weight ("0.1" for "<0.1title:@groonga") after the weight's scope is finished ("0.1" in "<0.1title:@groonga") and the next weight ("0.3" in "<0.3content:@Full") is popped after it's pushed. So the next condition ("content:@Full") uses the first weight ("0.1") not the second weight ("0.3").

Example (| is the current position):

  • |<0.1title:@Groonga <0.3content:@full: initial state: (current stack: [0.0])
  • <0.1|title:@Groonga <0.3content:@full: push 0.1: (current stack: [0.0, 0.1 (= 0.0 + 0.1 to support nested adjust)])
  • <0.1title:@Groonga| <0.3content:@full: title:@Groonga uses 0.1 as weight: (current stack: [0.0, 0.1])
  • <0.1title:@Groonga <0.3|content:@full: push 0.3: (current stack: [0.0, 0.1, 0.4 (= 0.1 + 0.3 to support nested adjust)])
  • <0.1title:@Groonga <0.3|content:@full: pop: (current stack: [0.0, 0.1])
  • <0.1title:@Groonga <0.3content:@full|: content:@full uses 0.1 (not 0.3) as weight: (current stack: [0.0, 0.1]))
  • <0.1title:@Groonga <0.3content:@full|: pop: (current stack: [0.0])

Therefore, the latest weight doesn't apply result.
As a result, the previous weight is applied multiple times.

With this change, multiple weights are applied correctly:

  • |<0.1title:@Groonga <0.3content:@full: initial state: (current stack: [0.0])
  • <0.1|title:@Groonga <0.3content:@full: push 0.1: (current stack: [0.0, 0.1 (= 0.0 + 0.1 to support nested adjust)])
  • <0.1title:@Groonga| <0.3content:@full: title:@Groonga uses 0.1 as weight: (current stack: [0.0, 0.1])
  • <0.1title:@Groonga <0.3|content:@full: pop: (current stack: [0.0])
  • <0.1title:@Groonga <0.3|content:@full: push 0.3: (current stack: [0.0, 0.3 (= 0.0 + 0.3 to support nested adjust)])
  • <0.1title:@Groonga <0.3content:@full|: content:@full uses 0.3) as weight: (current stack: [0.0, 0.3]))
  • <0.1title:@Groonga <0.3content:@full|: pop: (current stack: [0.0])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multiple adjusts use wrong weight
2 participants