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

Add InnerHits functionality #1172

Merged
merged 5 commits into from
Sep 9, 2016
Merged

Add InnerHits functionality #1172

merged 5 commits into from
Sep 9, 2016

Conversation

wamania
Copy link
Contributor

@wamania wamania commented Sep 8, 2016

Add the innerHits functionality as described in
https://www.elastic.co/guide/en/elasticsearch/reference/2.4/search-request-inner-hits.html
for Nested aggregation and Child/parent relations

@ruflin
Copy link
Owner

ruflin commented Sep 8, 2016

Very nice addition. Could you also update the CHANGELOG.md file with this?

@wamania
Copy link
Contributor Author

wamania commented Sep 8, 2016

Done !

*
* @return $this
*/
public function setScriptFields($scriptFields)
Copy link
Owner

Choose a reason for hiding this comment

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

Should we require ScriptFields type here? One can just use new ScriptsFileds(...) when passing the argument. Like this we can verify the type and have less "magic".

Copy link
Contributor Author

@wamania wamania Sep 8, 2016

Choose a reason for hiding this comment

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

I used the same structure as Elastica\Aggregation\TopHits which is very similar.

/**
     * Set script fields.
     *
     * @param array|\Elastica\Script\ScriptFields $scriptFields
     *
     * @return $this
     */
    public function setScriptFields($scriptFields)
    {
        if (is_array($scriptFields)) {
            $scriptFields = new ScriptFields($scriptFields);
        }
        return $this->setParam('script_fields', $scriptFields);
    }

I think we should keep the same prototype (we can change both).

Copy link
Owner

Choose a reason for hiding this comment

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

I think the scriptFields in the TopHits is probably for historical reasons as it was perhaps introduced before ScriptFields and only arrays were passed.

Unfortunately we can't remove it from TopHits directly as it would break BC. We should add a note there that it is deprecated. Even though it is not consistent I prefer not to build up more legacy and only use ScriptFields here to be supported. Otherwise there will be in more BC breaks if we remove it one day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, i agree

@ruflin
Copy link
Owner

ruflin commented Sep 8, 2016

LGTM. Only left one minor comment. Very nice functional tests you added.

@wamania
Copy link
Contributor Author

wamania commented Sep 8, 2016

look good now, argument forced to type ScriptFields, test ok

@ruflin ruflin merged commit 1c49fd8 into ruflin:master Sep 9, 2016
@ruflin
Copy link
Owner

ruflin commented Sep 9, 2016

@wamania Merged. Thanks a lot for the addition.

@wamania
Copy link
Contributor Author

wamania commented Sep 9, 2016

Happy to have been usefull, tell me if i can do some thing else.

@ruflin
Copy link
Owner

ruflin commented Sep 12, 2016

Help is always welcome :-) The biggest task ahead of Elastica currently from my perspective is: #944 Already several people played around with different approaches (including me) but I think we didn't get to a final conclusion yet. More then happy to hear your thoughts on this one.

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