Query "exists" in Latest not supported #1266

Closed
Zyqsempai opened this Issue Mar 9, 2017 · 13 comments

Comments

Projects
None yet
4 participants
@Zyqsempai
Contributor

Zyqsempai commented Mar 9, 2017

Hi there, i got this error "query exists in Latest not supported", from here https://github.com/ruflin/Elastica/blob/master/lib/Elastica/QueryBuilder/Facade.php#L55 ,but real cause lays here https://github.com/ruflin/Elastica/blob/master/lib/Elastica/QueryBuilder/Version/Version240.php#L15 few methods from Query are missing in $queries array.
We have few ways to solve this problem.
1)First and easiest is just to add missing methods to $queries array.
2)Second method is pretty easy too, we can add new Version, with missing methods.
But i got the feeling that we doing something completely wrong by storing methods names statically in array, same for aggregations and suggestions DSL.
Thats why i think that we need to get methods names dynamically from their classes.
What do you think, which method do you prefer, point me and i will make PR.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 11, 2017

Owner

We should probably take a more general look at this problem as the query builder is kind of unmaintained. As Elastica and ES are now in sync again related to versions I'm not sure if it makes sense to keep the "Version" feature in place but only support the version it is for and remove all other methods. I remember @Tobion also had some thoughts about this.

If we need a quick solution, we should probably go for option 1. But if we have the time, lets take a step back and have a general look at the query builder future. More thoughts on this would be appreciated, especially as I never really use the query builder myself and I'm interested to hear more in the use cases around it.

Owner

ruflin commented Mar 11, 2017

We should probably take a more general look at this problem as the query builder is kind of unmaintained. As Elastica and ES are now in sync again related to versions I'm not sure if it makes sense to keep the "Version" feature in place but only support the version it is for and remove all other methods. I remember @Tobion also had some thoughts about this.

If we need a quick solution, we should probably go for option 1. But if we have the time, lets take a step back and have a general look at the query builder future. More thoughts on this would be appreciated, especially as I never really use the query builder myself and I'm interested to hear more in the use cases around it.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 12, 2017

Collaborator

I'd prefer to deprecate the QueryBuilder as it's intention to support many ES version within Elastica is not how Elastica is maintained at the moment.
Also a QueryBuilder is usually there to build a query with a fluent interface and with a shorter notation like in Doctrine. But the elastica queries already have a fluent interface themselves and there is nothing to shorten. So using the QueryBuilder doesn't bring any advantage.

@Zyqsempai could you please give us examples how you use the QueryBuilder just to be sure we're not forgetting a use-case.

Collaborator

Tobion commented Mar 12, 2017

I'd prefer to deprecate the QueryBuilder as it's intention to support many ES version within Elastica is not how Elastica is maintained at the moment.
Also a QueryBuilder is usually there to build a query with a fluent interface and with a shorter notation like in Doctrine. But the elastica queries already have a fluent interface themselves and there is nothing to shorten. So using the QueryBuilder doesn't bring any advantage.

@Zyqsempai could you please give us examples how you use the QueryBuilder just to be sure we're not forgetting a use-case.

@Zyqsempai

This comment has been minimized.

Show comment
Hide comment
@Zyqsempai

Zyqsempai Mar 13, 2017

Contributor

@ruflin I have an opposite position, i always use QueryBuilder, one of the main reason, is what @Tobion mentioned above, it's really cool to have same interfaces for DB and ES queries, and our current QB really have many common things with Doctrine QB and it's really safe for team and project to have such a simplicity, i can be sure that some one new for ES have an intuitive way to edit and create new queries.
I think that many of Elastica users choosed Elastica because of QB. And it will be a big mistake to remove that functionality, but i agree that we need to refactor it, first of all as @ruflin mentioned we can remove Version layer QB mast be syncronized to Elastica current version there is no way to keep few versions of QB.
Also, since i always used QB and never native Elastica queries i admit that maybe i am wrong)))
Here is a simply example of QB query in my project, how it will looks like with native query?

$this->query = $qb->query()->bool()->addShould(
                                $qb->query()->function_score()->setBoost(100)->setQuery(
                                    $qb->query()->term()->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                $qb->query()->function_score()->setBoost(20)->setQuery(
                                    $qb->query()->term()->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery.'*')
                            );
Contributor

Zyqsempai commented Mar 13, 2017

@ruflin I have an opposite position, i always use QueryBuilder, one of the main reason, is what @Tobion mentioned above, it's really cool to have same interfaces for DB and ES queries, and our current QB really have many common things with Doctrine QB and it's really safe for team and project to have such a simplicity, i can be sure that some one new for ES have an intuitive way to edit and create new queries.
I think that many of Elastica users choosed Elastica because of QB. And it will be a big mistake to remove that functionality, but i agree that we need to refactor it, first of all as @ruflin mentioned we can remove Version layer QB mast be syncronized to Elastica current version there is no way to keep few versions of QB.
Also, since i always used QB and never native Elastica queries i admit that maybe i am wrong)))
Here is a simply example of QB query in my project, how it will looks like with native query?

$this->query = $qb->query()->bool()->addShould(
                                $qb->query()->function_score()->setBoost(100)->setQuery(
                                    $qb->query()->term()->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                $qb->query()->function_score()->setBoost(20)->setQuery(
                                    $qb->query()->term()->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery.'*')
                            );
@Zyqsempai

This comment has been minimized.

Show comment
Hide comment
@Zyqsempai

Zyqsempai Mar 14, 2017

Contributor

There are no response from you guys, so i will make simple PR with actions from step (1), that will "relieve tension" for some time.

Contributor

Zyqsempai commented Mar 14, 2017

There are no response from you guys, so i will make simple PR with actions from step (1), that will "relieve tension" for some time.

Zyqsempai added a commit to Zyqsempai/Elastica that referenced this issue Mar 14, 2017

- Fix for QueryBuilder version check `\Elastica\QueryBuilder\Version\…
…Version240.php` added all new query types to queries array. #1266
@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 15, 2017

Owner

Looking at your examples above I agree that we should keep the query builder, but clean it up. We must make sure it is also tested to if we change things they don't break.

@Zyqsempai Will merge PR #1269 and looking forward to more contributions related to QB ;-)

Owner

ruflin commented Mar 15, 2017

Looking at your examples above I agree that we should keep the query builder, but clean it up. We must make sure it is also tested to if we change things they don't break.

@Zyqsempai Will merge PR #1269 and looking forward to more contributions related to QB ;-)

ruflin added a commit that referenced this issue Mar 15, 2017

- Fix for QueryBuilder version check `\Elastica\QueryBuilder\Version\…
…Version240.php` added all new query types to queries array. #1266 (#1269)
@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 16, 2017

Collaborator

The query builder example you gave

$this->query = $qb->query()->bool()->addShould(
                                $qb->query()->function_score()->setBoost(100)->setQuery(
                                    $qb->query()->term()->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                $qb->query()->function_score()->setBoost(20)->setQuery(
                                    $qb->query()->term()->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery.'*')
                            );

translates to without the query builder:

$this->query = (new BoolQuery())->addShould(
                                (new FunctionScore())->setBoost(100)->setQuery(
                                    (new Term())->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                (new FunctionScore())->setBoost(20)->setQuery(
                                   (new Term())->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', '*'.$clearQuery.'*')
                            );

So instead of calling a method you just instantiate a class. It's even shorter. The query builder doesn't add any value.

Collaborator

Tobion commented Mar 16, 2017

The query builder example you gave

$this->query = $qb->query()->bool()->addShould(
                                $qb->query()->function_score()->setBoost(100)->setQuery(
                                    $qb->query()->term()->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                $qb->query()->function_score()->setBoost(20)->setQuery(
                                    $qb->query()->term()->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                $qb->query()->wildcard()->setValue('number.clean', '*'.$clearQuery.'*')
                            );

translates to without the query builder:

$this->query = (new BoolQuery())->addShould(
                                (new FunctionScore())->setBoost(100)->setQuery(
                                    (new Term())->setTerm('number.raw', $rawQuery)
                                )
                            )->addShould(
                                (new FunctionScore())->setBoost(20)->setQuery(
                                   (new Term())->setTerm('number.clean', $clearQuery)
                                )
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', $clearQuery.'*', '2.0')
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', '*'.$clearQuery, '1.5')
                            )->addShould(
                                (new Wildcard())->setValue('number.clean', '*'.$clearQuery.'*')
                            );

So instead of calling a method you just instantiate a class. It's even shorter. The query builder doesn't add any value.

@Zyqsempai

This comment has been minimized.

Show comment
Hide comment
@Zyqsempai

Zyqsempai Mar 16, 2017

Contributor

@Tobion it all depends on point of view or maybe just style preference, i think that this is great that we have that kind of choise in our library, i described above the reasons why i prefer that style, and i don't see anything bad in query builder, and i will take care of it from now.

Contributor

Zyqsempai commented Mar 16, 2017

@Tobion it all depends on point of view or maybe just style preference, i think that this is great that we have that kind of choise in our library, i described above the reasons why i prefer that style, and i don't see anything bad in query builder, and i will take care of it from now.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 16, 2017

Collaborator

Having multiple choices without any difference is bad. Bad to maintain, bad to document, bad for consistency, bad for learning. You said yourself you don't know the other style, which shows exactly the problems.

Collaborator

Tobion commented Mar 16, 2017

Having multiple choices without any difference is bad. Bad to maintain, bad to document, bad for consistency, bad for learning. You said yourself you don't know the other style, which shows exactly the problems.

@Zyqsempai

This comment has been minimized.

Show comment
Hide comment
@Zyqsempai

Zyqsempai Mar 16, 2017

Contributor

Sorry, but who and why decided that query builder is a bad practice, for me it's a very good example of incapsulation in right way and in right place, don't forget that Doctrine keep it in that way, and you don't say that Ocramius is wrong here, for me your style are very primitive and not intuitive for new users, but i don't try to convince you, so and you just try to think a little bit different, Elastica lived with those two ways many years without any problem, and we will keep it in that way.

Contributor

Zyqsempai commented Mar 16, 2017

Sorry, but who and why decided that query builder is a bad practice, for me it's a very good example of incapsulation in right way and in right place, don't forget that Doctrine keep it in that way, and you don't say that Ocramius is wrong here, for me your style are very primitive and not intuitive for new users, but i don't try to convince you, so and you just try to think a little bit different, Elastica lived with those two ways many years without any problem, and we will keep it in that way.

@shandyDev

This comment has been minimized.

Show comment
Hide comment
@shandyDev

shandyDev Mar 17, 2017

I agree with @Tobion that it is necessary to further maintenance & support on which need additional efforts. But @Zyqsempai also right - it is great, provide additional choose for developers. For example, code above with "new" (imagine that this is a class method) can not mocking normally and therefore hard testing because of hardcoded dependencies. And querybuilder methods can mock with return mocks for each elastica queries.
So I think we can keep querybuilder and clean up it. For example delete Versions functional (because each elastica version support own elasticsearch version), make querybuilder more fluent (without Facade), etc.

I agree with @Tobion that it is necessary to further maintenance & support on which need additional efforts. But @Zyqsempai also right - it is great, provide additional choose for developers. For example, code above with "new" (imagine that this is a class method) can not mocking normally and therefore hard testing because of hardcoded dependencies. And querybuilder methods can mock with return mocks for each elastica queries.
So I think we can keep querybuilder and clean up it. For example delete Versions functional (because each elastica version support own elasticsearch version), make querybuilder more fluent (without Facade), etc.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 17, 2017

Collaborator

Why would you want to mock the query? You mock requests and responses, but never queries.

Collaborator

Tobion commented Mar 17, 2017

Why would you want to mock the query? You mock requests and responses, but never queries.

@ruflin

This comment has been minimized.

Show comment
Hide comment
@ruflin

ruflin Mar 17, 2017

Owner

Let's make sure we do not discuss in this thread about good or bad coding practices because that is something which each engineer has its own perspective on. There are quite a few users out there which use the query build which is great. They way I personally position the query builder is for people that know the json representation in ES of a query and want to quickly write it in php (kind of).

The main thing I worry about query builder is the maintenance part. How much overhead is it to keep maintaining it? How can we assure it keeps working / is tested? Cleaning up the query builder code seems to me a good step in the right direction.

Owner

ruflin commented Mar 17, 2017

Let's make sure we do not discuss in this thread about good or bad coding practices because that is something which each engineer has its own perspective on. There are quite a few users out there which use the query build which is great. They way I personally position the query builder is for people that know the json representation in ES of a query and want to quickly write it in php (kind of).

The main thing I worry about query builder is the maintenance part. How much overhead is it to keep maintaining it? How can we assure it keeps working / is tested? Cleaning up the query builder code seems to me a good step in the right direction.

@Tobion

This comment has been minimized.

Show comment
Hide comment
@Tobion

Tobion Mar 25, 2017

Collaborator

Closing as #1269 has been merged

Collaborator

Tobion commented Mar 25, 2017

Closing as #1269 has been merged

@Tobion Tobion closed this Mar 25, 2017

mhernik pushed a commit to mhernik/Elastica that referenced this issue Jul 24, 2017

- Fix for QueryBuilder version check `\Elastica\QueryBuilder\Version\…
…Version240.php` added all new query types to queries array. #1266 (#1269)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment