From ffb76aae9ebdfd47991732aaa11458063e8e15f6 Mon Sep 17 00:00:00 2001 From: Vincent Hagen Date: Fri, 30 Jun 2023 21:50:58 +0200 Subject: [PATCH 1/2] Rework rescoring to be query property --- src/Domain/Query/Query.php | 46 +++++++++++-------- .../Query/QueryProperties/Combinable.php | 13 ++++++ .../Query/QueryProperties/Rescorers.php | 35 ++++++++++++++ .../Query/{ => QueryProperties}/Rescoring.php | 11 ++++- tests/Unit/Query/QueryTest.php | 19 ++++---- tests/Unit/Syntax/RescoringTest.php | 5 +- 6 files changed, 96 insertions(+), 33 deletions(-) create mode 100644 src/Domain/Query/QueryProperties/Combinable.php create mode 100644 src/Domain/Query/QueryProperties/Rescorers.php rename src/Domain/Query/{ => QueryProperties}/Rescoring.php (86%) diff --git a/src/Domain/Query/Query.php b/src/Domain/Query/Query.php index 2cb292a0..d91f2e47 100644 --- a/src/Domain/Query/Query.php +++ b/src/Domain/Query/Query.php @@ -5,10 +5,13 @@ namespace JeroenG\Explorer\Domain\Query; use JeroenG\Explorer\Domain\Aggregations\AggregationSyntaxInterface; +use JeroenG\Explorer\Domain\Query\QueryProperties\Combinable; use JeroenG\Explorer\Domain\Query\QueryProperties\QueryProperty; -use JeroenG\Explorer\Domain\Query\QueryProperties\SourceFilter; +use JeroenG\Explorer\Domain\Query\QueryProperties\Rescorers; +use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring; use JeroenG\Explorer\Domain\Syntax\Sort; use JeroenG\Explorer\Domain\Syntax\SyntaxInterface; +use Webmozart\Assert\Assert; class Query implements SyntaxInterface { @@ -16,9 +19,6 @@ class Query implements SyntaxInterface private ?int $limit = null; - /** @var Rescoring[] */ - private array $rescoring = []; - private array $fields = []; /** @var Sort[] */ @@ -61,10 +61,6 @@ public function build(): array $query['fields'] = $this->fields; } - if ($this->hasRescoring()) { - $query['rescore'] = $this->buildRescoring(); - } - if ($this->hasAggregations()) { $query['aggs'] = array_map( fn (AggregationSyntaxInterface $value) => $value->build(), @@ -72,12 +68,9 @@ public function build(): array ); } - $allQueryProperties = array_map( - static fn (QueryProperty $queryProperties) => $queryProperties->build(), - $this->queryProperties - ); + $queryProperties = $this->buildQueryProperties(); - return array_merge($query, ...$allQueryProperties); + return array_merge($query, ...$queryProperties); } public function setOffset(?int $offset): void @@ -112,7 +105,7 @@ public function addQueryProperties(QueryProperty ...$properties): void public function addRescoring(Rescoring $rescoring): void { - $this->rescoring[] = $rescoring; + $this->queryProperties[] = Rescorers::for($rescoring); } public function addAggregation(string $name, AggregationSyntaxInterface $aggregationItem): void @@ -150,13 +143,26 @@ private function buildSort(): array return array_map(static fn ($item) => $item->build(), $this->sort); } - private function hasRescoring(): bool + private function buildQueryProperties(): array { - return !empty($this->rescoring); - } + /** @var array> $allCombinables */ + $allCombinables = []; + $allQueryProperties = []; - private function buildRescoring(): array - { - return array_map(fn (Rescoring $rescore) => $rescore->build(), $this->rescoring); + foreach ($this->queryProperties as $queryProperty) { + if ($queryProperty instanceof Combinable) { + $allCombinables[get_class($queryProperty)] = $allCombinables[get_class($queryProperty)] ?? []; + $allCombinables[get_class($queryProperty)][] = $queryProperty; + } else { + $allQueryProperties[] = $queryProperty; + } + } + + /** @var array $sameCombinables */ + foreach ($allCombinables as $sameCombinables) { + $allQueryProperties[] = $sameCombinables[0]->combine(...array_slice($sameCombinables, 1)); + } + + return array_map(static fn (QueryProperty $queryProperty) => $queryProperty->build(), $allQueryProperties); } } diff --git a/src/Domain/Query/QueryProperties/Combinable.php b/src/Domain/Query/QueryProperties/Combinable.php new file mode 100644 index 00000000..de8e1c41 --- /dev/null +++ b/src/Domain/Query/QueryProperties/Combinable.php @@ -0,0 +1,13 @@ + array_map(static fn (Rescoring $rescoring) => $rescoring->build(), $this->rescoringQueries), + ]; + } + + public function combine(...$self): self + { + $all = []; + foreach($self as $rescorer) { + array_push($all, ...$this->rescoringQueries, ...$rescorer->rescoringQueries); + } + + return new self($all); + } +} \ No newline at end of file diff --git a/src/Domain/Query/Rescoring.php b/src/Domain/Query/QueryProperties/Rescoring.php similarity index 86% rename from src/Domain/Query/Rescoring.php rename to src/Domain/Query/QueryProperties/Rescoring.php index d635737f..7aea87d5 100644 --- a/src/Domain/Query/Rescoring.php +++ b/src/Domain/Query/QueryProperties/Rescoring.php @@ -2,11 +2,11 @@ declare(strict_types=1); -namespace JeroenG\Explorer\Domain\Query; +namespace JeroenG\Explorer\Domain\Query\QueryProperties; use JeroenG\Explorer\Domain\Syntax\SyntaxInterface; -class Rescoring implements SyntaxInterface +class Rescoring { public const SCORE_MODE_TOTAL = 'total'; @@ -28,6 +28,13 @@ class Rescoring implements SyntaxInterface private SyntaxInterface $query; + public static function create(SyntaxInterface $query): self + { + $rescore = new self(); + $rescore->query = $query; + return $rescore; + } + public function build(): array { return [ diff --git a/tests/Unit/Query/QueryTest.php b/tests/Unit/Query/QueryTest.php index 5d1a05d5..429d43c9 100644 --- a/tests/Unit/Query/QueryTest.php +++ b/tests/Unit/Query/QueryTest.php @@ -6,10 +6,11 @@ use JeroenG\Explorer\Domain\Aggregations\TermsAggregation; use JeroenG\Explorer\Domain\Query\Query; +use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring; use JeroenG\Explorer\Domain\Query\QueryProperties\SourceFilter; -use JeroenG\Explorer\Domain\Query\Rescoring; use JeroenG\Explorer\Domain\Syntax\MatchAll; use JeroenG\Explorer\Domain\Syntax\Sort; +use JeroenG\Explorer\Domain\Syntax\Term; use PHPUnit\Framework\TestCase; final class QueryTest extends TestCase @@ -86,18 +87,21 @@ public function test_it_builds_query_with_fields(): void public function test_it_builds_query_with_rescoring(): void { - $rescoring = new Rescoring(); - $rescoring->setQuery(new MatchAll()); - $this->query->addRescoring($rescoring); - $this->query->addRescoring($rescoring); + $rescoring1 = new Rescoring(); + $rescoring1->setQuery(new MatchAll()); + $this->query->addRescoring($rescoring1); + + $rescoring2 = new Rescoring(); + $rescoring2->setQuery(new Term(':fld:')); + $this->query->addRescoring($rescoring2); $result = $this->query->build(); self::assertEquals([ 'query' => ['match_all' => (object)[]], 'rescore' => [ - $rescoring->build(), - $rescoring->build() + $rescoring1->build(), + $rescoring2->build(), ] ], $result); } @@ -136,6 +140,5 @@ public function test_it_builds_with_source_filter_query_property(): void 'exclude' => $exclude, ] ], $this->query->build()); - } } diff --git a/tests/Unit/Syntax/RescoringTest.php b/tests/Unit/Syntax/RescoringTest.php index f6e87da6..17a0d5b8 100644 --- a/tests/Unit/Syntax/RescoringTest.php +++ b/tests/Unit/Syntax/RescoringTest.php @@ -4,7 +4,7 @@ namespace JeroenG\Explorer\Tests\Unit\Syntax; -use JeroenG\Explorer\Domain\Query\Rescoring; +use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring; use JeroenG\Explorer\Domain\Syntax\MatchAll; use PHPUnit\Framework\TestCase; @@ -31,8 +31,7 @@ public function test_it_builds_rescoring_query_with_defaults(): void public function test_it_builds_rescoring_query_with_properties(): void { - $rescoring = new Rescoring(); - $rescoring->setQuery(new MatchAll()); + $rescoring = Rescoring::create(new MatchAll()); $rescoring->setScoreMode(Rescoring::SCORE_MODE_MULTIPLY); $rescoring->setQueryWeight(2); $rescoring->setRescoreQueryWeight(42); From 93db134b80119877c9566e65e8a62a3a30e20437 Mon Sep 17 00:00:00 2001 From: Vincent Hagen Date: Sat, 1 Jul 2023 15:55:54 +0200 Subject: [PATCH 2/2] Rework Sorting to Combinable QueryProperties. - Found a failure in sorting which also effected rescoring - Add explicit tests for both - Standardized construction for both Rescoring and Sorting --- src/Domain/Query/Query.php | 27 +++---- .../Query/QueryProperties/Rescorers.php | 7 +- src/Domain/Query/QueryProperties/Sorting.php | 42 +++++++++++ src/Domain/Syntax/Sort.php | 2 +- .../Query/QueryProperties/RescorersTest.php | 72 +++++++++++++++++++ .../Query/QueryProperties/SortingTest.php | 50 +++++++++++++ tests/Unit/FakeResponseTest.php | 13 ---- tests/Unit/Query/QueryTest.php | 20 ++++++ 8 files changed, 197 insertions(+), 36 deletions(-) create mode 100644 src/Domain/Query/QueryProperties/Sorting.php create mode 100644 tests/Unit/Domain/Query/QueryProperties/RescorersTest.php create mode 100644 tests/Unit/Domain/Query/QueryProperties/SortingTest.php diff --git a/src/Domain/Query/Query.php b/src/Domain/Query/Query.php index d91f2e47..ada57562 100644 --- a/src/Domain/Query/Query.php +++ b/src/Domain/Query/Query.php @@ -9,6 +9,7 @@ use JeroenG\Explorer\Domain\Query\QueryProperties\QueryProperty; use JeroenG\Explorer\Domain\Query\QueryProperties\Rescorers; use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring; +use JeroenG\Explorer\Domain\Query\QueryProperties\Sorting; use JeroenG\Explorer\Domain\Syntax\Sort; use JeroenG\Explorer\Domain\Syntax\SyntaxInterface; use Webmozart\Assert\Assert; @@ -21,9 +22,6 @@ class Query implements SyntaxInterface private array $fields = []; - /** @var Sort[] */ - private array $sort = []; - /** @var QueryProperty[] */ private array $queryProperties = []; @@ -53,10 +51,6 @@ public function build(): array $query['size'] = $this->limit; } - if ($this->hasSort()) { - $query['sort'] = $this->buildSort(); - } - if ($this->hasFields()) { $query['fields'] = $this->fields; } @@ -90,7 +84,14 @@ public function setFields(array $fields): void public function setSort(array $sort): void { - $this->sort = $sort; + $this->queryProperties = array_filter( + $this->queryProperties, + static fn (QueryProperty $queryProperty) => !($queryProperty instanceof Sorting) + ); + + if (count($sort) > 0) { + $this->queryProperties[] = Sorting::for(...$sort); + } } public function setQuery(SyntaxInterface $query): void @@ -128,21 +129,11 @@ private function hasSize(): bool return !is_null($this->limit); } - private function hasSort(): bool - { - return !empty($this->sort); - } - private function hasFields(): bool { return !empty($this->fields); } - private function buildSort(): array - { - return array_map(static fn ($item) => $item->build(), $this->sort); - } - private function buildQueryProperties(): array { /** @var array> $allCombinables */ diff --git a/src/Domain/Query/QueryProperties/Rescorers.php b/src/Domain/Query/QueryProperties/Rescorers.php index 975ca0d0..1c700c70 100644 --- a/src/Domain/Query/QueryProperties/Rescorers.php +++ b/src/Domain/Query/QueryProperties/Rescorers.php @@ -4,7 +4,7 @@ namespace JeroenG\Explorer\Domain\Query\QueryProperties; -class Rescorers implements QueryProperty, Combinable +final class Rescorers implements QueryProperty, Combinable { private function __construct( private array $rescoringQueries = [], @@ -17,7 +17,6 @@ public static function for(Rescoring ...$rescoring): self public function build(): array { - return [ 'rescore' => array_map(static fn (Rescoring $rescoring) => $rescoring->build(), $this->rescoringQueries), ]; @@ -25,9 +24,9 @@ public function build(): array public function combine(...$self): self { - $all = []; + $all = $this->rescoringQueries; foreach($self as $rescorer) { - array_push($all, ...$this->rescoringQueries, ...$rescorer->rescoringQueries); + array_push($all, ...$rescorer->rescoringQueries); } return new self($all); diff --git a/src/Domain/Query/QueryProperties/Sorting.php b/src/Domain/Query/QueryProperties/Sorting.php new file mode 100644 index 00000000..a92ef7d7 --- /dev/null +++ b/src/Domain/Query/QueryProperties/Sorting.php @@ -0,0 +1,42 @@ +sorts; + + /** @var Sorting[] $self */ + foreach($self as $sorting) { + array_push($all, ...$sorting->sorts); + } + + return new self($all); + } + + public function build(): array + { + return [ + 'sort' => array_map( + static fn (Sort $sort) => $sort->build(), + $this->sorts, + ), + ]; + } +} \ No newline at end of file diff --git a/src/Domain/Syntax/Sort.php b/src/Domain/Syntax/Sort.php index 3e3b0f78..125b18d9 100644 --- a/src/Domain/Syntax/Sort.php +++ b/src/Domain/Syntax/Sort.php @@ -6,7 +6,7 @@ use Webmozart\Assert\Assert; -class Sort implements SyntaxInterface +class Sort { public const ASCENDING = 'asc'; diff --git a/tests/Unit/Domain/Query/QueryProperties/RescorersTest.php b/tests/Unit/Domain/Query/QueryProperties/RescorersTest.php new file mode 100644 index 00000000..2ac96173 --- /dev/null +++ b/tests/Unit/Domain/Query/QueryProperties/RescorersTest.php @@ -0,0 +1,72 @@ + [self::rescoreQueryPart(':fld:', ':val:')]], $sort->build()); + } + + public function test_it_combines(): void + { + $a = Rescorers::for( + Rescoring::create(new Term(':fld1:', ':val1:')), + Rescoring::create(new Term(':fld2:', ':val2:')), + ); + $b = Rescorers::for( + Rescoring::create(new Term(':fld3:', ':val3:')), + Rescoring::create(new Term(':fld4:', ':val4:')), + ); + $c = Rescorers::for( + Rescoring::create(new Term(':fld5:', ':val5:')), + ); + $d = Rescorers::for(); + + $result = $a->combine($b, $c, $d); + + self::assertNotSame($a->build(), $result->build()); + self::assertSame([ + 'rescore' => [ + self::rescoreQueryPart(':fld1:', ':val1:'), + self::rescoreQueryPart(':fld2:', ':val2:'), + self::rescoreQueryPart(':fld3:', ':val3:'), + self::rescoreQueryPart(':fld4:', ':val4:'), + self::rescoreQueryPart(':fld5:', ':val5:'), + ], + ], $result->build()); + } + + private static function rescoreQueryPart(string $fld, string $val): array + { + return [ + 'window_size' => 10, + 'query' => [ + 'score_mode' => 'total', + 'rescore_query' => [ + 'term' => [ + $fld => [ + 'value' => $val, + 'boost' => 1.0, + ] + ] + ], + 'query_weight' => 1.0, + 'rescore_query_weight' => 1.0, + ] + ]; + } +} diff --git a/tests/Unit/Domain/Query/QueryProperties/SortingTest.php b/tests/Unit/Domain/Query/QueryProperties/SortingTest.php new file mode 100644 index 00000000..5f1dfaf0 --- /dev/null +++ b/tests/Unit/Domain/Query/QueryProperties/SortingTest.php @@ -0,0 +1,50 @@ + [ [ ':fld:' => 'desc' ]]],$sort->build()); + } + + public function test_it_combines(): void + { + $a = Sorting::for( + new Sort(':fld1:', Sort::DESCENDING), + new Sort(':fld2:', Sort::DESCENDING), + ); + $b = Sorting::for( + new Sort(':fld3:', Sort::DESCENDING), + new Sort(':fld4:', Sort::DESCENDING), + ); + $c = Sorting::for( + new Sort(':fld5:', Sort::DESCENDING), + ); + $d = Sorting::for(); + + $result = $a->combine($b, $c, $d); + + self::assertNotSame($a->build(), $result->build()); + self::assertSame([ + 'sort' => [ + [ ':fld1:' => 'desc' ], + [ ':fld2:' => 'desc' ], + [ ':fld3:' => 'desc' ], + [ ':fld4:' => 'desc' ], + [ ':fld5:' => 'desc' ], + ], + ], $result->build()); + } +} diff --git a/tests/Unit/FakeResponseTest.php b/tests/Unit/FakeResponseTest.php index a102c3b0..182c7527 100644 --- a/tests/Unit/FakeResponseTest.php +++ b/tests/Unit/FakeResponseTest.php @@ -4,21 +4,8 @@ namespace JeroenG\Explorer\Tests\Unit; -use Elasticsearch\Client; use InvalidArgumentException; -use JeroenG\Explorer\Application\AggregationResult; -use JeroenG\Explorer\Application\SearchCommand; -use JeroenG\Explorer\Domain\Aggregations\TermsAggregation; -use JeroenG\Explorer\Domain\Query\Query; -use JeroenG\Explorer\Domain\Syntax\Compound\BoolQuery; -use JeroenG\Explorer\Domain\Syntax\Matching; -use JeroenG\Explorer\Domain\Syntax\Sort; -use JeroenG\Explorer\Domain\Syntax\Term; -use JeroenG\Explorer\Infrastructure\Elastic\ElasticClientFactory; use JeroenG\Explorer\Infrastructure\Elastic\FakeResponse; -use JeroenG\Explorer\Infrastructure\Elastic\Finder; -use JeroenG\Explorer\Infrastructure\Scout\ScoutSearchCommandBuilder; -use Mockery; use Mockery\Adapter\Phpunit\MockeryTestCase; class FakeResponseTest extends MockeryTestCase diff --git a/tests/Unit/Query/QueryTest.php b/tests/Unit/Query/QueryTest.php index 429d43c9..408a0f19 100644 --- a/tests/Unit/Query/QueryTest.php +++ b/tests/Unit/Query/QueryTest.php @@ -8,10 +8,12 @@ use JeroenG\Explorer\Domain\Query\Query; use JeroenG\Explorer\Domain\Query\QueryProperties\Rescoring; use JeroenG\Explorer\Domain\Query\QueryProperties\SourceFilter; +use JeroenG\Explorer\Domain\Query\QueryProperties\TrackTotalHits; use JeroenG\Explorer\Domain\Syntax\MatchAll; use JeroenG\Explorer\Domain\Syntax\Sort; use JeroenG\Explorer\Domain\Syntax\Term; use PHPUnit\Framework\TestCase; +use TypeError; final class QueryTest extends TestCase { @@ -43,6 +45,24 @@ public function test_it_builds_query_with_sort(): void self::assertEquals([$sort->build()], $result['sort'] ?? null); } + public function test_it_throws_on_invalid_sort_argument(): void + { + $this->expectException(TypeError::class); + $this->query->setSort([new Term(':fld:', ':val:')]); + } + + public function test_it_reset_sort(): void + { + $this->query->addQueryProperties(TrackTotalHits::all()); + $this->query->setSort([new Sort(':fld:')]); + $this->query->setSort([]); + + $result = $this->query->build(); + + self::assertArrayNotHasKey('sort', $result); + self::assertArrayHasKey('track_total_hits', $result); + } + public function test_it_builds_query_with_pagination(): void { $this->query->setLimit(10);