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

Field Collapsing Support #1653

Merged
merged 4 commits into from Aug 22, 2019
Merged

Field Collapsing Support #1653

merged 4 commits into from Aug 22, 2019

Conversation

@wackerl91
Copy link
Contributor

@wackerl91 wackerl91 commented Aug 20, 2019

Added support for Field Collapsing as requested in #1392.
ES documentation: https://www.elastic.co/guide/en/elasticsearch/reference/current/search-request-body.html#request-body-search-collapse

Note: Inheriting Query\InnerHits in Collapse\InnerHits was something I've been debating with myself over and over again. As far as I'm concerned it makes sense to have different classes, especially as the collapse one has to have support for the second level collapse, but I didn't want to duplicate the entire code for InnerHits.
Downside is that right now there's no AbstractCollapse base-class as there is for the other concepts. Still feel like that's okay, because field collapsing is a quite simple concept right now.

Copy link
Owner

@ruflin ruflin left a comment

Thanks for the contribution, over LGTM. Also appreciate the details on why inheriting innerhits or going with abstract. I guess we can still change it later without breaking the API if we figure out the other path works better?

There are a few changes in this PR where I'm not sure if it's cleanup or related to the PR. If the addition of self etc. is not related, I think it's great to have it in but I would prefer to have it in a separate PR to separate the two and make the diff smaller.

{
return $this->setParam('query', $query);
}

/**
* Gets the query object.
*
* @return \Elastica\Query\AbstractQuery
* @return array|\Elastica\Query\AbstractQuery
Copy link
Owner

@ruflin ruflin Aug 21, 2019

Choose a reason for hiding this comment

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

Where is this change coming from?

Copy link
Contributor Author

@wackerl91 wackerl91 Aug 21, 2019

Choose a reason for hiding this comment

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

Both Query::create and Query::__construct allow passing in an array as argument, which don't parse / convert this array into an ensemble of objects but keep it as-is, essentially leveraging the setRawQuery method.

Calling getQuery after taking the route above (or by just setting a raw query on the Query object) will return an array. I think that's something one should be aware of.

FWIW, I do like the option to set the raw query, however I'd rather have it in a way where it's not mingled with the usual query abstraction but has it's own representation to have better control over it during it's lifecycle.

@@ -86,7 +91,7 @@ public static function create($query)
*
* @return $this
*/
public function setRawQuery(array $query)
public function setRawQuery(array $query): self
Copy link
Owner

@ruflin ruflin Aug 21, 2019

Choose a reason for hiding this comment

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

Is this needed or some cleanup?

Copy link
Contributor Author

@wackerl91 wackerl91 Aug 21, 2019

Choose a reason for hiding this comment

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

Cleanup. I'll remove them later today and re-add them in a new PR as you requested in your comment above.

@wackerl91
Copy link
Contributor Author

@wackerl91 wackerl91 commented Aug 21, 2019

I guess we can still change it later without breaking the API if we figure out the other path works better?

Yes, that should work. Thing is that this might lead to a lot of duplicated code between both versions of InnerHits, on the flip side offering better control if they ever start to drift apart in ES DSL. I'd personally sit that out and wait how ES continues to handle field collapsing in the future, as this decision likely also depends on how complex collapsing becomes.

ruflin
ruflin approved these changes Aug 22, 2019
Copy link
Owner

@ruflin ruflin left a comment

SGTM. Thanks for all the tests!

@ruflin ruflin merged commit 37822e6 into ruflin:master Aug 22, 2019
1 check passed
ruflin added a commit that referenced this issue Dec 9, 2019
That's going to be a bit awkward now: in the PR that added Field Collapsing Support (#1653) I forgot to check in the DSL classes, which made working with it a bit clumsy compared to the usual query methods.
I.e. - as with aggregations - it's now possible to do stuff like this:
```
$collapse
  ->setFieldname('product_id')
  ->setInnerHits(
    $qb->collapse->inner_hits()  // instead of (new Collapse\InnerHits())
  )
```

Let me know if there need to be any changes. Cheers :)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants