Skip to content

Commit

Permalink
bug #269 [Elasticsearch] merge nested queries with the same path (jka…
Browse files Browse the repository at this point in the history 
…bat)

This PR was merged into the 2.0-dev branch.

Discussion
----------

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | #268 
| License       | MIT
| Doc PR        | 

Follow up to #268 

Current implementation merges only single level nested fields (eg: author[].first_name, author[].last_name), not multiple level ones (item[].author[].name...). 

@dkarlovi Any thoughts how this can be solved? Merging is not working for HAS_CHILD queries with multiple levels as well. 

As this PR solves my current requirement I'm fine with single level fix, but I agree that complete solution is preferred. Unit test covering multiple level nesting case is included but failing... can be removed later.

Commits
-------

1cf8cd9 bug #268 fix: merge single level nested queries with the same path
  • Loading branch information
@sstok
sstok authored Jun 5, 2019
2 parents 27a8990 + 1cf8cd9 commit 63f7dbf
Show file tree
Hide file tree
Showing 2 changed files with 200 additions and 26 deletions.
103 changes: 77 additions & 26 deletions lib/Elasticsearch/QueryConditionGenerator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -579,45 +579,68 @@ private function injectParameters($template)

private function injectConditions(array $query, array $conditions): array
{
if ([] !== $conditions) {
$hasChild = null;

if (isset($query[self::QUERY_HAS_CHILD])) {
// wrap has_child.query into a bool.must to prepare for accepting conditions
$query[self::QUERY_HAS_CHILD][self::QUERY] = [
self::QUERY_BOOL => [
self::CONDITION_AND => [
$query[self::QUERY_HAS_CHILD][self::QUERY],
],
if ([] === $conditions) {
return $query ?? [];
}

$childType = null;
$nestedPath = null;

if (isset($query[self::QUERY_HAS_CHILD])) {
// wrap has_child.query into a bool.must to prepare for accepting conditions
$query[self::QUERY_HAS_CHILD][self::QUERY] = [
self::QUERY_BOOL => [
self::CONDITION_AND => [
$query[self::QUERY_HAS_CHILD][self::QUERY],
],
];
$hasChild = $query[self::QUERY_HAS_CHILD][self::QUERY_TYPE];
$nestedBool = &$query[self::QUERY_HAS_CHILD][self::QUERY][self::QUERY_BOOL];
}
],
];

$query = [
$childType = $query[self::QUERY_HAS_CHILD][self::QUERY_TYPE];
$nestedBool = &$query[self::QUERY_HAS_CHILD][self::QUERY][self::QUERY_BOOL];
} elseif (isset($query[self::QUERY_NESTED])) {
// wrap nested.query into a bool.must to prepare for accepting conditions
$query[self::QUERY_NESTED][self::QUERY] = [
self::QUERY_BOOL => [
self::CONDITION_AND => [
$query,
$query[self::QUERY_NESTED][self::QUERY],
],
],
];
$rootBool = &$query[self::QUERY_BOOL];

foreach ($conditions as $condition) {
if (isset($condition[self::QUERY_HAS_CHILD])) {
if ($hasChild === $condition[self::QUERY_HAS_CHILD][self::QUERY_TYPE]) {
$this->mergeQuery($nestedBool, self::CONDITION_AND, $condition[self::QUERY_HAS_CHILD][self::QUERY]);
} else {
$this->mergeQuery($rootBool, self::CONDITION_AND, $condition);
}

$nestedPath = $query[self::QUERY_NESTED][self::QUERY_PATH];
$nestedBool = &$query[self::QUERY_NESTED][self::QUERY][self::QUERY_BOOL];
}

$query = [
self::QUERY_BOOL => [
self::CONDITION_AND => [
$query,
],
],
];

$rootBool = &$query[self::QUERY_BOOL];

foreach ($conditions as $condition) {
if (isset($condition[self::QUERY_HAS_CHILD])) {
if ($childType === $condition[self::QUERY_HAS_CHILD][self::QUERY_TYPE]) {
$this->mergeQuery($nestedBool, self::CONDITION_AND, $condition[self::QUERY_HAS_CHILD][self::QUERY]);
} else {
$this->mergeQuery($rootBool, self::CONDITION_AND, $condition);
}
} elseif (isset($condition[self::QUERY_NESTED])) {
if ($nestedPath === $condition[self::QUERY_NESTED][self::QUERY_PATH]) {
$this->mergeQuery($nestedBool, self::CONDITION_AND, $condition[self::QUERY_NESTED][self::QUERY]);
} else {
$this->mergeQuery($rootBool, self::CONDITION_AND, $condition);
}
} else {
$this->mergeQuery($rootBool, self::CONDITION_AND, $condition);
}
}

return $query ?? [];
return $query;
}

/**
Expand Down Expand Up @@ -687,6 +710,34 @@ private function mergeQuery(array &$bool, string $condition, array $query)
}
}
unset($previousQuery);
} elseif (isset($query[self::QUERY_NESTED]) && isset($bool[$condition])) {
foreach ($bool[$condition] as &$previousQuery) {
if (isset($previousQuery[self::QUERY_NESTED])
&& $previousQuery[self::QUERY_NESTED][self::QUERY_PATH] === $query[self::QUERY_NESTED][self::QUERY_PATH]) {
$previousQuery[self::QUERY_NESTED] = array_replace(
$previousQuery[self::QUERY_NESTED],
$query[self::QUERY_NESTED],
[
self::QUERY => $previousQuery[self::QUERY_NESTED][self::QUERY],
]
);

if (!isset($previousQuery[self::QUERY_NESTED][self::QUERY][self::QUERY_BOOL])) {
$previousQuery[self::QUERY_NESTED][self::QUERY] = [
self::QUERY_BOOL => [
$condition => [
$previousQuery[self::QUERY_NESTED][self::QUERY],
],
],
];
}

$previousQuery[self::QUERY_NESTED][self::QUERY][self::QUERY_BOOL][$condition][] = $query[self::QUERY_NESTED][self::QUERY];

return;
}
}
unset($previousQuery);
}

$bool[$condition][] = $query;
Expand Down
123 changes: 123 additions & 0 deletions lib/Elasticsearch/Tests/QueryConditionGeneratorTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1045,6 +1045,129 @@ public function it_merges_has_child_queries_from_conditional_order_queries()
self::assertMapping(['@date'], $generator->getMappings());
}

/** @test */
public function it_merges_single_level_nested_queries()
{
$condition = $this->createCondition()
->field('name')
->addSimpleValue('Doctor')
->addSimpleValue('Foo')
->end()
->field('restrict')
->addSimpleValue('Some')
->addSimpleValue('Restriction')
->end()
->getSearchCondition();

$generator = new QueryConditionGenerator($condition);
$generator->registerField('name', 'author[].name');
$generator->registerField('restrict', 'author[].restrict');

self::assertEquals([
'query' => [
'bool' => [
'must' => [
[
'nested' => [
'path' => 'author',
'query' => [
'bool' => [
'must' => [
[
'terms' => [
'author.name' => [
'Doctor',
'Foo',
],
],
],
[
'terms' => [
'author.restrict' => [
'Some',
'Restriction',
],
],
],
],
],
],
],
],
],
],
],
], $generator->getQuery()->toArray());

self::assertMapping(['name', 'restrict'], $generator->getMappings());
}

/** @test */
public function it_merges_multiple_level_nested_queries()
{
$this->markTestIncomplete(
'This test has not been implemented yet. Read #268.'
);

$condition = $this->createCondition()
->field('name')
->addSimpleValue('Doctor')
->addSimpleValue('Foo')
->end()
->field('restrict')
->addSimpleValue('Some')
->addSimpleValue('Restriction')
->end()
->getSearchCondition();

$generator = new QueryConditionGenerator($condition);
$generator->registerField('name', 'item[].author[].name');
$generator->registerField('restrict', 'item[].author[].restrict');

self::assertEquals([
'query' => [
'bool' => [
'must' => [
[
'nested' => [
'path' => 'item',
'query' => [
'nested' => [
'path' => 'author',
'query' => [
'bool' => [
'must' => [
[
'terms' => [
'author.name' => [
'Doctor',
'Smith',
],
],
],
[
'terms' => [
'author.restrict' => [
'Some',
'Restriction',
],
],
],
],
],
],
],
],
],
],
],
],
],
], $generator->getQuery()->toArray());

self::assertMapping(['name', 'restrict'], $generator->getMappings());
}

protected function getFieldSet(bool $build = true, bool $order = false)
{
$fieldSet = parent::getFieldSet(false);
Expand Down

0 comments on commit 63f7dbf

Please sign in to comment.