Support fields in search mvp #1184

dranikpg · 2023-05-05T20:11:56Z

No description provided.

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg · 2023-05-05T20:14:15Z

src/core/search/base.h

+// Represents input to a search tree.
+struct SearchInput {
+  // Callback that's supplied with field values.
+  using FieldConsumer = std::function<bool(std::string_view)>;
+
+  virtual ~SearchInput() = default;
+
+  // Check if the given callback returns true on any of  the active fields.
+  virtual bool Check(FieldConsumer) = 0;
+
+  // Sets a single active field.
+  virtual void SelectField(std::string_view field) = 0;
+


Not sure about the interface yet.

Virtual so its fully mockable. Otherwise search needs to include core structures like StringMap and redis skiplists (or should it?)

I also don't like the FieldConsumer function glue here, maybe have a separate virtual base for nodes that work directly with strings

I'm not a fan of the fact that input is now a mutable argument to all expression checkers, but I don't have a better idea (nor do I fully understand the challenges here, tbh).
Since this is supposed to be a quick-N-dirty test ground, it's great to let it evolve as you go. If you do want to brainstorm about this - lmk, I'd be happy to!

Agree. Yes, this part won't outlive the basic MVP. Actually speaking returning a vector<string> is also an option because we do bruteforce search either way and performance is already not our goal. Once we do indexed, all out interfaces will change.

But lets still brainstorm quickly 🙂

Mutable state

The only state we have is the field (and I'm almost sure there won't be more). So if your query has @field:(subtree), then the whole subtree needs to be applied only to a specific field of the document, whereas by default they apply to all fields. I do this field shift with a single node that sets the active field.

The better idea for now would be most likely smth like this:

// Interface for accessing hset values with different data structures underneath. struct HSetAccessor { using FieldConsumer = std::function<bool(std::string_view)>; virtual bool Check(FieldConsumer f, std::string_view active_field = {}) const; }; struct SearchInput { const HSetAccessor* hset; std::string_view active_field; };

So now its all const and state is decoupled from the interface

FieldConsumer

Some context about the future: Why not fill an container like vector<string_view>*

The data structure can't always provide string_views to all its entries. With listpack some might be stored as real integers and need to be decoded to a temporary buffer (so we also cant return a single string_view, only if a buffer is passed). Keeping a separate buffer is cumbersome.

What I don't like with the FieldConsumer is that:

The iteration logic is handled by the data structure accessor

The node has to wrap its logic into one more layer

So I'd make the HSetAccessor return just some kind of virtual iterator. C++ ones are a pain really. Go style isn't bad.

struct HSetIterator { virtual string_view Next(); virtual bool HasNext(): }

So iteration logic can be performed by the nodes themselves... But it'll be the same for all of them. Keeping it in a middle-part like SearchInput as a helper requires one more virtual base.

dranikpg · 2023-05-05T20:16:22Z

src/core/search/parser.y

+field_cond_expr:
+  LPAREN field_cond_expr RPAREN                   { $$ = $2; }
+  | field_cond_expr field_cond_expr %prec AND_OP  { $$ = MakeExpr<AstLogicalNode>($1, $2, AstLogicalNode::kAnd); }
+  | field_cond_expr OR_OP field_cond_expr         { $$ = MakeExpr<AstLogicalNode>($1, $3, AstLogicalNode::kOr); }
+  | NOT_OP field_cond_expr                        { $$ = MakeExpr<AstNegateNode>($2); };
+  | TERM                                          { $$ = MakeExpr<AstTermNode>($1); }


Looks very similar to the tree above - all except the last case. Those need to be different symbols as its context free and there is no way to re-use it directly. But maybe there are macros and such in bison.

I have no idea if there are macros, but thanks for the explanation!
I have never seen bison code(?), so this helped me :)

chakaz · 2023-05-05T20:59:54Z

src/core/search/search_parser_test.cc

+  EXPECT_TRUE(Check(&input));
+
+  input = {{"f1", "foo"}, {"f2", "bar"}, {"f3", "last is wrong"}};
+  EXPECT_FALSE(Check(&input));


Can you also add a test case where f1 and f2 have the right values, but f3 is missing (instead of mismatching)?
(I realize it's similar to the last one, but not identical)

Its line 263, though I added other words to f1, but it should still match. I can remove them for clarity

chakaz · 2023-05-05T21:05:30Z

src/core/search/search_parser_test.cc

+  MockedSearchInput(std::string test_field) : hset_{{"field", test_field}} {
+  }
+
+  MockedSearchInput& operator=(Map hset) {


I tend to dislike implicit-looking conversions such as this. It's also against the Google style guide.
Can you please use a traditional method, like SetFrom() or whatever?

Ok, I'll do.

I just always thought the rules in the style guide are there for a reason. This one mainly to not have ambiguous or unexpected conversions/behaviour/etc with re-useable core components, as its easy to miss for others. Its clear where the danger is for large code bases.

I don't see any way to misuse it here inside tests 😅 I just want it to be readable and contain as few visual clutter (brackets) as possible.

chakaz · 2023-05-05T21:13:37Z

src/core/search/parser.y

+field_cond_expr:
+  LPAREN field_cond_expr RPAREN                   { $$ = $2; }
+  | field_cond_expr field_cond_expr %prec AND_OP  { $$ = MakeExpr<AstLogicalNode>($1, $2, AstLogicalNode::kAnd); }
+  | field_cond_expr OR_OP field_cond_expr         { $$ = MakeExpr<AstLogicalNode>($1, $3, AstLogicalNode::kOr); }
+  | NOT_OP field_cond_expr                        { $$ = MakeExpr<AstNegateNode>($2); };
+  | TERM                                          { $$ = MakeExpr<AstTermNode>($1); }


I have no idea if there are macros, but thanks for the explanation!
I have never seen bison code(?), so this helped me :)

chakaz · 2023-05-05T21:16:07Z

src/core/search/base.h

+// Represents input to a search tree.
+struct SearchInput {
+  // Callback that's supplied with field values.
+  using FieldConsumer = std::function<bool(std::string_view)>;
+
+  virtual ~SearchInput() = default;
+
+  // Check if the given callback returns true on any of  the active fields.
+  virtual bool Check(FieldConsumer) = 0;
+
+  // Sets a single active field.
+  virtual void SelectField(std::string_view field) = 0;
+


I'm not a fan of the fact that input is now a mutable argument to all expression checkers, but I don't have a better idea (nor do I fully understand the challenges here, tbh).
Since this is supposed to be a quick-N-dirty test ground, it's great to let it evolve as you go. If you do want to brainstorm about this - lmk, I'd be happy to!

romange · 2023-05-06T06:49:44Z

Fyi, i am not sure that listpack is going to meet search any time soon. Not that i think you are wrong in general

…

On Sat, May 6, 2023, 09:42 Vladislav ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/core/search/base.h <#1184 (comment)> : > +// Represents input to a search tree. +struct SearchInput { + // Callback that's supplied with field values. + using FieldConsumer = std::function<bool(std::string_view)>; + + virtual ~SearchInput() = default; + + // Check if the given callback returns true on any of the active fields. + virtual bool Check(FieldConsumer) = 0; + + // Sets a single active field. + virtual void SelectField(std::string_view field) = 0; + Agree. Yes, this part won't outlive the MVP, but lets still brainstorm quickly 🙂 1. Mutable state The only state we have is the field (and I'm almost sure there won't be more). So if your query has @field:(subtree), then the whole subtree needs to be applied only to a specific field of the document, whereas by default they apply to all fields. I do this field shift with a single node that sets the active field. The better idea for now would be most likely smth like this: // Interface for accessing hset values with different data structures underneath.struct HSetAccessor { using FieldConsumer = std::function<bool(std::string_view)>; virtual bool Check(FieldConsumer f, std::string_view active_field = {}) const; }; struct SearchInput { const HSetAccessor* hset; std::string_view active_field; }; So now its all const and state is decoupled from the interface. The downside I see in the future is that with explicit SelectField/ClearField the real implementation can store an iterator/pointer and look up this value again much faster, as it could be used in a large subtree. But it can also do it as an internal optimization, comparing last accessed values. 1. FieldConsumer Some context about the future: Why not fill an container like vector<string_view>* - The data structure can't always provide string_views to all its entries. With listpack some might be stored as real integers and need to be decoded to a temporary buffer (so we also cant return a single string_view, only if a buffer is passed) - We probably don't want to iterate over all fields if we match on the first What I don't like with the FieldConsumer is that: - The iteration logic is handled by the data structure accessor - The node has to wrap its logic into one more layer So I'd make the HSetAccessor return just some kind of virtual iterator. C++ ones are a pain really. Go style isn't bad. struct HSetIterator { virtual string_view Next(); virtual bool HasNext(): } So iteration logic can be performed by the nodes themselves... But it'll be the same for all of them. Keeping it in a middle-part like SearchInput as a helper requires one more virtual base. — Reply to this email directly, view it on GitHub <#1184 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4BFCFAJMRBPZLTW4VAH3TXEXXFPANCNFSM6AAAAAAXXRJALA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

romange · 2023-05-06T06:52:56Z

Styleguide are also for managing larger teams, not only for large codebases. A junior that joins will be confused if he sees that the line is blurry.

…

On Sat, May 6, 2023, 09:49 Roman Gershman ***@***.***> wrote: Fyi, i am not sure that listpack is going to meet search any time soon. Not that i think you are wrong in general On Sat, May 6, 2023, 09:42 Vladislav ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In src/core/search/base.h > <#1184 (comment)> > : > > > +// Represents input to a search tree. > +struct SearchInput { > + // Callback that's supplied with field values. > + using FieldConsumer = std::function<bool(std::string_view)>; > + > + virtual ~SearchInput() = default; > + > + // Check if the given callback returns true on any of the active fields. > + virtual bool Check(FieldConsumer) = 0; > + > + // Sets a single active field. > + virtual void SelectField(std::string_view field) = 0; > + > > Agree. Yes, this part won't outlive the MVP, but lets still brainstorm > quickly 🙂 > > 1. Mutable state > > The only state we have is the field (and I'm almost sure there won't be > more). So if your query has @field:(subtree), then the whole subtree > needs to be applied only to a specific field of the document, whereas by > default they apply to all fields. I do this field shift with a single node > that sets the active field. > > The better idea for now would be most likely smth like this: > > // Interface for accessing hset values with different data structures underneath.struct HSetAccessor { > using FieldConsumer = std::function<bool(std::string_view)>; > virtual bool Check(FieldConsumer f, std::string_view active_field = {}) const; > }; > struct SearchInput { > const HSetAccessor* hset; > std::string_view active_field; > }; > > So now its all const and state is decoupled from the interface. The > downside I see in the future is that with explicit SelectField/ClearField > the real implementation can store an iterator/pointer and look up this > value again much faster, as it could be used in a large subtree. But it can > also do it as an internal optimization, comparing last accessed values. > > 1. FieldConsumer > > Some context about the future: Why not fill an container like > vector<string_view>* > > - The data structure can't always provide string_views to all its > entries. With listpack some might be stored as real integers and need to be > decoded to a temporary buffer (so we also cant return a single string_view, > only if a buffer is passed) > - We probably don't want to iterate over all fields if we match on > the first > > What I don't like with the FieldConsumer is that: > > - The iteration logic is handled by the data structure accessor > - The node has to wrap its logic into one more layer > > So I'd make the HSetAccessor return just some kind of virtual iterator. > C++ ones are a pain really. Go style isn't bad. > > struct HSetIterator { > virtual string_view Next(); > virtual bool HasNext(): > } > > So iteration logic can be performed by the nodes themselves... But it'll > be the same for all of them. Keeping it in a middle-part like SearchInput > as a helper requires one more virtual base. > > — > Reply to this email directly, view it on GitHub > <#1184 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AA4BFCFAJMRBPZLTW4VAH3TXEXXFPANCNFSM6AAAAAAXXRJALA> > . > You are receiving this because you are subscribed to this thread.Message > ID: ***@***.***> >

dranikpg · 2023-05-06T06:55:09Z

Fyi, i am not sure that listpack is going to meet search any time soon. Not
that i think you are wrong in general

Aren't hashes stored with listpack as well?

romange · 2023-05-06T07:59:22Z

oh, I forgot about this small map optimization. You are right.

dranikpg

Fixed comments

dranikpg · 2023-05-06T16:49:49Z

src/core/search/base.h

+// Interface for accessing hashset values with different data structures underneath.
+struct HSetAccessor {
+  // Callback that's supplied with field values.
+  using FieldConsumer = std::function<bool(std::string_view)>;
+
+  virtual bool Check(FieldConsumer f, std::string_view active_field) const = 0;
+};
+
+// Wrapper around hashset accessor and optional active field.
+struct SearchInput {
+  SearchInput(const HSetAccessor* hset, std::string_view active_field = {})
+      : hset_{hset}, active_field_{active_field} {
+  }
+
+  SearchInput(const SearchInput& base, std::string_view active_field)
+      : hset_{base.hset_}, active_field_{active_field} {
+  }
+
+  bool Check(HSetAccessor::FieldConsumer f) {
+    return hset_->Check(move(f), active_field_);
+  }
+
+ private:
+  const HSetAccessor* hset_;
+  std::string_view active_field_;
+};


Lets keep the structure like this (open to comments about the code). Anyways indexed search requires another interface, so its all temporary

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

chakaz · 2023-05-06T17:37:08Z

LGTM! It won't let me merge (never actually done it in GitHub so I might miss something).

dranikpg · 2023-05-06T18:23:35Z

My changes dismissed your review + it was still a draft

chakaz · 2023-05-06T18:26:30Z

src/core/search/ast_expr.h

+class AstFieldNode : public AstNode {
+ public:
+  AstFieldNode(std::string field, NodePtr node) : field_{field.substr(1)}, node_{node} {
+  }


Consider adding a CHECK() that field[0] == '@'

Field support for search mvp

feat: support fields in search mvp

a733349

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg commented May 5, 2023

View reviewed changes

dranikpg requested a review from chakaz May 5, 2023 20:28

chakaz previously approved these changes May 5, 2023

View reviewed changes

dranikpg dismissed chakaz’s stale review via 2859d92 May 6, 2023 15:29

dranikpg commented May 6, 2023

View reviewed changes

fix: fixes

6f7f082

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

dranikpg force-pushed the basic-rsearch-fields branch from 2859d92 to 6f7f082 Compare May 6, 2023 16:51

dranikpg marked this pull request as ready for review May 6, 2023 18:22

dranikpg requested a review from chakaz May 6, 2023 18:23

chakaz approved these changes May 6, 2023

View reviewed changes

dranikpg merged commit dc853fe into dragonflydb:main May 6, 2023
6 checks passed

dranikpg deleted the basic-rsearch-fields branch May 28, 2023 09:17

romange pushed a commit that referenced this pull request Jun 1, 2023

Support fields in search mvp (#1184)

1df98bd

Field support for search mvp

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fields in search mvp #1184

Support fields in search mvp #1184

dranikpg commented May 5, 2023

dranikpg May 5, 2023

chakaz May 5, 2023

dranikpg May 6, 2023 •

edited

dranikpg May 5, 2023

chakaz May 5, 2023

chakaz May 5, 2023

dranikpg May 6, 2023

chakaz May 5, 2023

dranikpg May 6, 2023 •

edited

chakaz May 5, 2023

chakaz May 5, 2023

romange commented May 6, 2023 via email

romange commented May 6, 2023 via email

dranikpg commented May 6, 2023

romange commented May 6, 2023

dranikpg left a comment

dranikpg May 6, 2023

chakaz May 6, 2023

chakaz commented May 6, 2023

dranikpg commented May 6, 2023

chakaz May 6, 2023

Support fields in search mvp #1184

Support fields in search mvp #1184

Conversation

dranikpg commented May 5, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dranikpg May 6, 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

Choose a reason for hiding this comment

dranikpg May 6, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romange commented May 6, 2023 via email

romange commented May 6, 2023 via email

dranikpg commented May 6, 2023

romange commented May 6, 2023

dranikpg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chakaz commented May 6, 2023

dranikpg commented May 6, 2023

Choose a reason for hiding this comment

dranikpg May 6, 2023 •

edited

dranikpg May 6, 2023 •

edited