Skip to content

Conversation

@mrliptontea
Copy link

Hi,

Currently, the bool operator preceding subquery is ignored, effectively causing this query:

"iPad Pro" AND (Gold OR Silver)

to be parsed as:

"iPad Pro" OR (Gold OR Silver)

which isn't correct.

I added $boolOperator argument back to subquery's constructor.
It should not break any API unless someone is manually instantiating Subquery in their code.

@gdbrown
Copy link
Contributor

gdbrown commented Dec 21, 2016

Thanks for the PR. I'll review it within a few days. First glance seems fine although I feel like we didn't do this on sub queries intentionally until some quirk with elasticsearch queries was figured out. I'll need to look at that aspect to make sure this won't create some issue there.

@mrliptontea
Copy link
Author

Hi. I'll start by saying this is excellent package, it greatly helped me achieve what I wanted in my project, thank you 😄

Let me explain my motivation for this change: in my code, I extended QueryParser getBoolOperator method to override the default value with "required". This way all terms in my query are joined with logical "and" unless a user explicitly specifies otherwise.

I think QueryParser should provide consistent implementation, isolated from quirks of any search engine/database it might be used with - that would be builder's job. In this example, on a Builder class level, you could ignore/override bool operator for subqueries, and achieve the same result.

Similarly, I think the following should be considered:

  • Number nodes don't have bool operators; that's fine in most cases, except when you change the default value for bool,
  • Emoji, Emoticon, Hashtag, and Mention nodes override "optional" with "required"; this should not happen, IMO, and it's the builder that could force that,
  • getBoolOperator method's implementation isn't complete - I had to re-implement it to change the default value, otherwise using "optional" wouldn't be possible. Adding conditions for "or" would be enough, and I could simply replace the default value for the method argument, and call the method on the parent.

@gdbrown gdbrown changed the base branch from master to bool-op-subquery December 28, 2016 19:28
@gdbrown gdbrown merged commit ad1e4d5 into gdbots:bool-op-subquery Dec 28, 2016
gdbrown added a commit that referenced this pull request Dec 28, 2016
gdbrown added a commit that referenced this pull request Dec 28, 2016
* Respect boolean operator preceding subquery (#9)

* Respect boolean operator preceding subquery

* Add test for negating subqueries

* pull #9 ensure fromArray brings in bool operator as well and update doc block and changelog
@gdbrown
Copy link
Contributor

gdbrown commented Dec 28, 2016

this was merged and released as https://github.com/gdbots/query-parser-php/releases/tag/v0.2.1

I also made an issue for the other notes... #11

we'll get to that next year. :)

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.

2 participants