Query builder #842

wickedOne · 2020-07-14T09:25:34Z

helper class to help writing and maintaining (filter) queries;
replaces #733

i had a look at the SearchApiSolrBackend::createFilterQuery method but concluded that the main difference between that class and this helper class is that the createFilterQuery method does a lot of "filling in the blanks" based on field types which is information not available in this class.

i did however add the Comparison::EMPTY constant and method to cover the edge case mentioned there twice for someone willing to query empty values. to keep things clean and simple, i've added a new comparison method rather than trying to incorporate it in others based on values passed.

also the addition two simple integrations in the query class Query::setQueryFromQueryBuilder and Query::addFilterQueriesFromQueryBuilder.

codecov-commenter · 2020-07-14T09:31:50Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.79%. Comparing base (636bfdb) to head (e6bd9b7).
Report is 173 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #842      +/-   ##
==========================================
+ Coverage   92.64%   92.79%   +0.15%     
==========================================
  Files         335      342       +7     
  Lines        8263     8435     +172     
==========================================
+ Hits         7655     7827     +172     
  Misses        608      608

Flag	Coverage Δ
unittests	`92.79% <100.00%> (+0.15%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

expression builder to ease the use of solr's query language

- added query builder implementation in query trait - added query builder implementation for adding filter queries - added some documentation for the query builder

thomascorthals · 2020-07-30T12:58:32Z

docs/queries/query-helper/query-builder.md

+Complex filter queries
+----------------------
+While the ``addFilterQueriesFromQueryBuilder`` method only provides in setting the facet query key and actual query, the ``QueryBuilder`` can be used in the construction of more complex facet queries.
+If one, for example, need to add a tag to the filter query the following method could be used.


Suggested change

If one, for example, need to add a tag to the filter query the following method could be used.

If one, for example, needs to add a tag to the filter query the following method could be used.

thomascorthals · 2020-07-30T20:28:13Z

src/Builder/Select/QueryExpressionVisitor.php

+     *
+     * @return string
+     */
+    private function typedValueToString($value, string $quote = '', $escape = true): string


One could theoretically wonder if $escape is nullable without a typehint.

Suggested change

private function typedValueToString($value, string $quote = '', $escape = true): string

private function typedValueToString($value, string $quote = '', bool $escape = true): string

thomascorthals · 2020-07-30T20:37:02Z

src/Builder/Select/QueryExpressionVisitor.php

+                return implode(' AND ', $comparisons);
+            case CompositeComparison::TYPE_OR:
+                return implode(' OR ', $comparisons);


Should we use the constants \Solarium\QueryType\Select\Query\Query::QUERY_OPERATOR_AND and QUERY_OPERATOR_OR instead? It seems a bit overkillish and I don't think it has any benefits. Just wondering if you had considered it and what made you decide against it.

thomascorthals · 2020-07-30T20:44:37Z

src/Builder/Select/QueryExpressionVisitor.php

+                }
+
+                return sprintf('%s:%s', $field, $this->valueToString($value, ',', '', false));
+            case Comparison::NIN:


I do have a strong opinion about the order of the cases. I think NIN should come right after IN.

Personally, I'd stick to the same order for all the cases as the consts in Comparison are defined in.

thomascorthals

Looks good as a whole. My remarks are mostly about readability rather than functionality.

thomascorthals · 2020-07-30T21:02:44Z

src/Builder/Select/ExpressionBuilder.php

+    /**
+     * @param null $x
+     *
+     * @throws \Solarium\Exception\RuntimeException
+     *
+     * @return \Solarium\Builder\CompositeComparison
+     */
+    public function andX($x = null): CompositeComparison
+    {
+        return new CompositeComparison(CompositeComparison::TYPE_AND, \func_get_args());
+    }


The ... operator makes this typehintable and thus more readable.

Suggested change

/**

* @param null $x

*

* @throws \Solarium\Exception\RuntimeException

*

* @return \Solarium\Builder\CompositeComparison

*/

public function andX($x = null): CompositeComparison

{

return new CompositeComparison(CompositeComparison::TYPE_AND, \func_get_args());

}

/**

* @param Comparison ...$comparison

*

* @throws \Solarium\Exception\RuntimeException

*

* @return \Solarium\Builder\CompositeComparison

*/

public function andX(Comparison ...$comparison): CompositeComparison

{

return new CompositeComparison(CompositeComparison::TYPE_AND, $comparison);

}

i've been working quite a bit with the FunctionBuilder recently and one of the benefits of not typehinting the arguments is that you can pass a string as argument for the edge cases you run into.

for instance creating this function filter(mult(price,quantity),or(cats:equal(category,_))) would translate into

$expression = FunctionBuilder::create() ->where($expr->filter($expr->mult('price', 'quantity'), $expr->or('cats:equal(category,_)'))) ;

as $expr->equals would render a different expression, it cannot be used to generate the desired result.

as this is a helper class, i'd personally prefer to make it not too strict; what do you think?

thomascorthals · 2020-07-30T21:03:55Z

src/Builder/Select/ExpressionBuilder.php

+    /**
+     * @param null $x
+     *
+     * @throws \Solarium\Exception\RuntimeException
+     *
+     * @return \Solarium\Builder\CompositeComparison
+     */
+    public function orX($x = null): CompositeComparison
+    {
+        return new CompositeComparison(CompositeComparison::TYPE_OR, \func_get_args());
+    }


Suggested change

/**

* @param null $x

*

* @throws \Solarium\Exception\RuntimeException

*

* @return \Solarium\Builder\CompositeComparison

*/

public function orX($x = null): CompositeComparison

{

return new CompositeComparison(CompositeComparison::TYPE_OR, \func_get_args());

}

/**

* @param Comparison ...$comparison

*

* @throws \Solarium\Exception\RuntimeException

*

* @return \Solarium\Builder\CompositeComparison

*/

public function orX(Comparison ...$comparison): CompositeComparison

{

return new CompositeComparison(CompositeComparison::TYPE_OR, $comparison);

}

thomascorthals · 2020-07-30T21:05:55Z

Sorry for messing that up as multiple comments instead of a single review!

wickedOne · 2020-07-30T21:23:58Z

@thomascorthals thanx for your review. i do agree with all of your suggestions.

as for the usage of constants: initially this was written to be a completely seperate component outside of the Solarium namespace thus no dependecies and it can be considered a leftover.

nevertheless i do prefer to have my constants defined in the namespace handling them for clarity.
but i'm fine either way...

thomascorthals · 2020-07-30T21:31:56Z

@wickedOne On the other hand, I don't think a hardcoded AND and OR will get us into trouble with Solr. If they change that syntax, we're probably in for one hell of a ride.

mkalkbrenner

We need to mention that this query builder must only be used in combination with the standard query parser (lucene) or edismax.

The default operator is "OR". The resulting string will be shorter if we use a single space as glue instead of " OR ".
And I prefer to do +a:1 +b:2instead of a:1 AND b:1.
This way you can reduce the length of the query string. This way you could avoid switching to POST requests more often and keep GET. For POST requests some of Solr's caches are disabled.

In general I consider the query builder to be useful. But we need to add documentation. Having a query with much boolean operators is slow. In most cases it makes a lot of sense to split the query in multiple filter queries which could be cached individually. So the examples should consist of code where we use this query builder to create a query in combination with multiple filter queries.

mkalkbrenner · 2020-08-12T13:23:46Z

tests/Builder/Select/QueryBuilderTest.php

+        $filter = QueryBuilder::create()
+            ->where(QueryBuilder::expr()->notIn('foo', 'bar'));
+
+        $this->assertSame('-foo:"bar"', $this->visitor->dispatch($filter->getExpressions()[0]));


In most cases you want *:* -foo:"bar"here.

wickedOne · 2020-12-27T08:36:27Z

We need to mention that this query builder must only be used in combination with the standard query parser (lucene) or edismax.

i think there's quite a few query parsers which also work with the standard query language?

The default operator is "OR". The resulting string will be shorter if we use a single space as glue instead of " OR ".
And I prefer to do +a:1 +b:2instead of a:1 AND b:1.
This way you can reduce the length of the query string. This way you could avoid switching to POST requests more often and keep GET. For POST requests some of Solr's caches are disabled.

fine with me, but are you sure that also works with the Lucene, Payload Score or Surround Query Parser to name a few?
perhaps better to instantiate the builder with the operators you want?

In general I consider the query builder to be useful. But we need to add documentation. Having a query with much boolean operators is slow. In most cases it makes a lot of sense to split the query in multiple filter queries which could be cached individually. So the examples should consist of code where we use this query builder to create a query in combination with multiple filter queries.

you think this is the responsibility of the solarium library? if so, we might want to consider a "best practices" section in the documentation (which i think would be very helpful) as these considerations are not limited to using a query builder

mkalkbrenner · 2020-12-29T09:23:36Z

We need to mention that this query builder must only be used in combination with the standard query parser (lucene) or edismax.

i think there's quite a few query parsers which also work with the standard query language?

Yes, but not all.

The default operator is "OR". The resulting string will be shorter if we use a single space as glue instead of " OR ". And I prefer to do +a:1 +b:2instead of a:1 AND b:1. This way you can reduce the length of the query string. This way you could avoid switching to POST requests more often and keep GET. For POST requests some of Solr's caches are disabled.

fine with me, but are you sure that also works with the Lucene, Payload Score or Surround Query Parser to name a few?
perhaps better to instantiate the builder with the operators you want?

It is the opposite. AND and OR are not accepted by all parsers. See https://lucene.apache.org/solr/guide/7_3/the-standard-query-parser.html#the-standard-query-parser

The standard query parser supports all the Boolean operators listed in the table above. The DisMax query parser supports only + and -.

In general I consider the query builder to be useful. But we need to add documentation. Having a query with much boolean operators is slow. In most cases it makes a lot of sense to split the query in multiple filter queries which could be cached individually. So the examples should consist of code where we use this query builder to create a query in combination with multiple filter queries.

you think this is the responsibility of the solarium library? if so, we might want to consider a "best practices" section in the documentation (which i think would be very helpful) as these considerations are not limited to using a query builder

You're right.

wickedOne · 2020-12-29T12:24:27Z

would you agree to a solution which by default uses + & - as operators which can be turned into AND and OR by using a flag on instantiation?

mkalkbrenner · 2020-12-29T12:31:50Z

Sure!

wickedOne added 5 commits July 19, 2020 17:54

select query builder

992e753

expression builder to ease the use of solr's query language

set timezone to circumvent helper bug

db2efd2

implementation and documentation

d05405c

- added query builder implementation in query trait - added query builder implementation for adding filter queries - added some documentation for the query builder

fixed invalid class name

2f66d19

style fixes

3d607d0

wickedOne force-pushed the query-builder branch from b8a1a22 to 3d607d0 Compare July 19, 2020 15:57

thomascorthals reviewed Jul 30, 2020

View reviewed changes

thomascorthals requested changes Jul 30, 2020

View reviewed changes

Merge branch 'master' into query-builder

1cadd40

mkalkbrenner requested changes Aug 12, 2020

View reviewed changes

Merge branch 'master' into query-builder

e6bd9b7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query builder #842

Query builder #842

wickedOne commented Jul 14, 2020

codecov-commenter commented Jul 14, 2020 •

edited by codecov bot

Loading

thomascorthals Jul 30, 2020

thomascorthals Jul 30, 2020

thomascorthals Jul 30, 2020

thomascorthals Jul 30, 2020

thomascorthals left a comment •

edited

Loading

thomascorthals Jul 30, 2020

wickedOne Dec 27, 2020

thomascorthals Jul 30, 2020

thomascorthals commented Jul 30, 2020

wickedOne commented Jul 30, 2020

thomascorthals commented Jul 30, 2020

mkalkbrenner left a comment

mkalkbrenner Aug 12, 2020

wickedOne commented Dec 27, 2020

mkalkbrenner commented Dec 29, 2020

wickedOne commented Dec 29, 2020

mkalkbrenner commented Dec 29, 2020

	If one, for example, need to add a tag to the filter query the following method could be used.
	If one, for example, needs to add a tag to the filter query the following method could be used.

	private function typedValueToString($value, string $quote = '', $escape = true): string
	private function typedValueToString($value, string $quote = '', bool $escape = true): string

Query builder #842

Are you sure you want to change the base?

Query builder #842

Conversation

wickedOne commented Jul 14, 2020

codecov-commenter commented Jul 14, 2020 • edited by codecov bot Loading

Codecov Report

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

thomascorthals left a comment • edited Loading

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

thomascorthals commented Jul 30, 2020

wickedOne commented Jul 30, 2020

thomascorthals commented Jul 30, 2020

mkalkbrenner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wickedOne commented Dec 27, 2020

mkalkbrenner commented Dec 29, 2020

wickedOne commented Dec 29, 2020

mkalkbrenner commented Dec 29, 2020

codecov-commenter commented Jul 14, 2020 •

edited by codecov bot

Loading

thomascorthals left a comment •

edited

Loading