Skip to content
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 26 additions & 15 deletions src/Adapter/MySQL.php
Original file line number Diff line number Diff line change
Expand Up @@ -310,18 +310,20 @@ protected function getFieldMapping()
'out_of_stock' => [
'tableName' => 'stock_available',
'tableAlias' => 'sa',
'joinCondition' => '(p.id_product = sa.id_product AND IFNULL(pac.id_product_attribute, 0) = sa.id_product_attribute' .
'joinCondition' => '(p.id_product = sa.id_product AND IFNULL(pa.id_product_attribute, 0) = sa.id_product_attribute' .
$stockCondition . ')',
'joinType' => self::LEFT_JOIN,
'dependencyField' => 'id_attribute',
'dependencyField' => 'id_product_attribute',
'shareOperationJoin' => true,
],
'quantity' => [
'tableName' => 'stock_available',
'tableAlias' => 'sa',
'joinCondition' => '(p.id_product = sa.id_product AND IFNULL(pac.id_product_attribute, 0) = sa.id_product_attribute' .
'joinCondition' => '(p.id_product = sa.id_product AND IFNULL(pa.id_product_attribute, 0) = sa.id_product_attribute' .
$stockCondition . ')',
'joinType' => self::LEFT_JOIN,
'dependencyField' => 'id_attribute',
'dependencyField' => 'id_product_attribute',
'shareOperationJoin' => true,
'aggregateFunction' => 'SUM',
'aggregateFieldName' => 'quantity',
],
Expand Down Expand Up @@ -708,9 +710,7 @@ protected function computeWhereConditions(array $filterToTableMapping)
$joinMapping = $filterToTableMapping[$operation[0]];
// If index is not the first, append to the table alias for
// multi join
$selectAlias = $joinMapping['tableAlias'] .
($operationIdx === 0 ? '' : '_' . $operationIdx) .
($idx === 0 ? '' : '_' . $idx);
$selectAlias = $this->getOperationTableAlias($joinMapping, $operationIdx, $idx);
$operation[0] = isset($joinMapping['fieldName']) ? $joinMapping['fieldName'] : $operation[0];
}

Expand Down Expand Up @@ -800,6 +800,20 @@ protected function computeWhereConditions(array $filterToTableMapping)
return $whereConditions;
}

/**
* @param array $joinMapping
* @param int $operationIdx
* @param int $idx
*
* @return string
*/
private function getOperationTableAlias(array $joinMapping, $operationIdx, $idx)
{
return $joinMapping['tableAlias'] .
($operationIdx === 0 ? '' : '_' . $operationIdx) .
($idx === 0 || !empty($joinMapping['shareOperationJoin']) ? '' : '_' . $idx);
}

/**
* Compute the joinConditions needed depending on the fields required in select, where, groupby & orderby fields
*
Expand All @@ -820,18 +834,15 @@ protected function computeJoinConditions(array $filterToTableMapping)
foreach ($operations as $idx => $operation) {
if ($this->requiresMappedTableForFilter($operation[0], $filterToTableMapping)) {
$joinMapping = $filterToTableMapping[$operation[0]];
if ($idx !== 0 || $operationIdx !== 0) {
$tableAlias = $this->getOperationTableAlias($joinMapping, $operationIdx, $idx);
if ($tableAlias !== $joinMapping['tableAlias']) {
// Index is not the first, append index to tableAlias on joinCondition
$joinMapping['joinCondition'] = preg_replace(
'~([\(\s=]' . $joinMapping['tableAlias'] . ')\.~',
'${1}' .
($operationIdx === 0 ? '' : '_' . $operationIdx) .
($idx === 0 ? '' : '_' . $idx) .
'.',
'~([\(\s=])' . $joinMapping['tableAlias'] . '\.~',
'${1}' . $tableAlias . '.',
$joinMapping['joinCondition']
);
$joinMapping['tableAlias'] .= ($operationIdx === 0 ? '' : '_' . $operationIdx) .
($idx === 0 ? '' : '_' . $idx);
$joinMapping['tableAlias'] = $tableAlias;
}

$this->addJoinConditions($joinList, $joinMapping, $filterToTableMapping);
Expand Down
137 changes: 89 additions & 48 deletions src/Filters/Block.php
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,9 @@ public function getFilterBlock(

// Get filters configured for the current query
$filters = $this->provider->getFiltersForQuery($this->query, $idShop);
$attributeValueCounts = $this->countAttributeValues($filters, $selectedFilters, $idLang);
$featureValueCounts = $this->countFeatureValues($filters, $selectedFilters, $idLang);
$this->dataAccessor->loadFeatureValues(array_keys(array_filter($featureValueCounts)), $idLang);

$filterBlocks = [];
// iterate through each filter, and the get corresponding filter block
Expand All @@ -143,11 +146,11 @@ public function getFilterBlock(
break;
case 'id_attribute_group':
$filterBlocks =
array_merge($filterBlocks, $this->getAttributesBlock($filter, $selectedFilters, $idLang));
array_merge($filterBlocks, $this->getAttributesBlock($filter, $selectedFilters, $idLang, $attributeValueCounts[$filter['id_value']] ?? []));
break;
case 'id_feature':
$filterBlocks =
array_merge($filterBlocks, $this->getFeaturesBlock($filter, $selectedFilters, $idLang));
array_merge($filterBlocks, $this->getFeaturesBlock($filter, $selectedFilters, $idLang, $featureValueCounts[$filter['id_value']] ?? []));
break;
case 'category':
$parent = new Category($idCategory, $idLang);
Expand Down Expand Up @@ -697,40 +700,22 @@ private function getManufacturersBlock($filter, $selectedFilters, $idLang)
* @param array $filter
* @param array $selectedFilters
* @param int $idLang
* @param array $results value counts of the attribute group
*
* @return array
*/
private function getAttributesBlock($filter, $selectedFilters, $idLang)
private function getAttributesBlock($filter, $selectedFilters, $idLang, array $results)
{
$attributesBlock = [];
$filteredSearchAdapter = null;
$idAttributeGroup = $filter['id_value'];

if (!empty($selectedFilters['id_attribute_group'])) {
foreach ($selectedFilters['id_attribute_group'] as $key => $selectedFilter) {
if ($key == $idAttributeGroup) {
$filteredSearchAdapter = $this->searchAdapter->getFilteredSearchAdapter('with_attributes_' . $idAttributeGroup);
break;
}
}
}

if (!$filteredSearchAdapter) {
$filteredSearchAdapter = $this->searchAdapter->getFilteredSearchAdapter();
}

$attributesGroup = $this->dataAccessor->getAttributesGroups($idLang);
if ($attributesGroup === []) {
return $attributesBlock;
}

$attributes = $this->dataAccessor->getAttributes($idLang, $idAttributeGroup);

$filteredSearchAdapter->addOperationsFilter(
'id_attribute_group_' . $idAttributeGroup,
[[['id_attribute_group', [(int) $idAttributeGroup]]]]
);
$results = $filteredSearchAdapter->valueCount('id_attribute');
foreach ($results as $key => $values) {
$idAttribute = $values['id_attribute'];
if (!isset($attributes[$idAttribute])) {
Expand Down Expand Up @@ -814,44 +799,20 @@ private function sortByKey(array $sortedReferenceArray, $array)
* @param array $filter
* @param array $selectedFilters
* @param int $idLang
* @param array $results value counts of the feature
*
* @return array
*/
private function getFeaturesBlock($filter, $selectedFilters, $idLang)
private function getFeaturesBlock($filter, $selectedFilters, $idLang, array $results)
{
$featureBlock = [];
$idFeature = $filter['id_value'];
$filteredSearchAdapter = null;

// Keep current selections while counting additional values for an AND feature facet.
if ((int) (isset($filter['filter_type']) ? $filter['filter_type'] : Converter::WIDGET_TYPE_CHECKBOX) !== Converter::WIDGET_TYPE_CHECKBOX_AND
&& !empty($selectedFilters['id_feature'])
) {
foreach ($selectedFilters['id_feature'] as $key => $selectedFilter) {
if ($key == $idFeature) {
$filteredSearchAdapter = $this->searchAdapter->getFilteredSearchAdapter('with_features_' . $idFeature);

break;
}
}
}

if (!$filteredSearchAdapter) {
$filteredSearchAdapter = $this->searchAdapter->getFilteredSearchAdapter();
}

$features = $this->dataAccessor->getFeatures($idLang);
if (empty($features)) {
return [];
}

$filteredSearchAdapter->addOperationsFilter(
'id_feature_' . $idFeature,
[[['id_feature', [(int) $idFeature]]]]
);

$filteredSearchAdapter->addSelectField('id_feature');
$results = $filteredSearchAdapter->valueCount('id_feature_value');
foreach ($results as $key => $values) {
$idFeatureValue = $values['id_feature_value'];
$idFeature = $values['id_feature'];
Expand Down Expand Up @@ -904,6 +865,85 @@ private function getFeaturesBlock($filter, $selectedFilters, $idLang)
return $featureBlock;
}

/**
* @return array<int, array> value counts by attribute group ID
*/
private function countAttributeValues(array $filters, array $selectedFilters, $idLang)
{
$sharedIds = $ownIds = [];
foreach ($filters as $filter) {
if ($filter['type'] === 'id_attribute_group') {
if (isset($selectedFilters['id_attribute_group'][$filter['id_value']])) {
$ownIds[] = (int) $filter['id_value'];
} else {
$sharedIds[] = (int) $filter['id_value'];
}
}
}

if ((empty($sharedIds) && empty($ownIds)) || $this->dataAccessor->getAttributesGroups($idLang) === []) {
return [];
}

return $this->countFacetValues($sharedIds, $ownIds, 'id_attribute_group', 'id_attribute', 'with_attributes_');
}

/**
* @return array<int, array> value counts by feature ID
*/
private function countFeatureValues(array $filters, array $selectedFilters, $idLang)
{
$sharedIds = $ownIds = [];
foreach ($filters as $filter) {
if ($filter['type'] === 'id_feature') {
// Keep current selections while counting additional values for an AND feature facet.
if ((int) ($filter['filter_type'] ?? Converter::WIDGET_TYPE_CHECKBOX) !== Converter::WIDGET_TYPE_CHECKBOX_AND
&& isset($selectedFilters['id_feature'][$filter['id_value']])
) {
$ownIds[] = (int) $filter['id_value'];
} else {
$sharedIds[] = (int) $filter['id_value'];
}
}
}

if ((empty($sharedIds) && empty($ownIds)) || empty($this->dataAccessor->getFeatures($idLang))) {
return [];
}

return $this->countFacetValues($sharedIds, $ownIds, 'id_feature', 'id_feature_value', 'with_features_');
}

/**
* A facet with a selected value ignores its own selection, so it is counted in its own query.
*
* @param int[] $sharedIds facets counted with the current filters
* @param int[] $ownIds facets ignoring their own selection
* @param string $idField id_attribute_group or id_feature
* @param string $valueField id_attribute or id_feature_value
* @param string $selectionFilterPrefix prefix of the filter holding the selection of a facet
*
* @return array<int, array> value counts by facet ID
*/
private function countFacetValues(array $sharedIds, array $ownIds, $idField, $valueField, $selectionFilterPrefix)
{
$queries = empty($sharedIds) ? [] : [[$this->searchAdapter->getFilteredSearchAdapter(), $sharedIds]];
foreach ($ownIds as $id) {
$queries[] = [$this->searchAdapter->getFilteredSearchAdapter($selectionFilterPrefix . $id), [$id]];
}

$valueCounts = array_fill_keys(array_merge($sharedIds, $ownIds), []);
foreach ($queries as list($filteredSearchAdapter, $ids)) {
$filteredSearchAdapter->addOperationsFilter($idField . '_' . implode('_', $ids), [[[$idField, $ids]]]);
$filteredSearchAdapter->addSelectField($idField);
foreach ($filteredSearchAdapter->valueCount($valueField) as $valueCount) {
$valueCounts[$valueCount[$idField]][] = $valueCount;
}
}

return $valueCounts;
}

/**
* Natural sort multi-dimensional feature array
*
Expand Down Expand Up @@ -959,6 +999,7 @@ private function addCategoriesBlockFilters(InterfaceAdapter $filteredSearchAdapt
}

$filteredSearchAdapter->addFilter('nleft', [$parent->nleft], '>');
$filteredSearchAdapter->addFilter('nleft', [$parent->nright], '<');
$filteredSearchAdapter->addFilter('nright', [$parent->nright], '<');
}

Expand Down
73 changes: 46 additions & 27 deletions src/Filters/DataAccessor.php
Original file line number Diff line number Diff line change
Expand Up @@ -205,37 +205,56 @@ public static function isFeatureValuePositionSupported()
public function getFeatureValues($idFeature, $idLang)
{
if (!isset($this->featureValues[$idLang][$idFeature])) {
// Initialize only the requested feature without discarding other cached features for the language.
$this->featureValues[$idLang][$idFeature] = [];

// feature_value.position only exists from PrestaShop 9.0 (PrestaShop/PrestaShop#37042),
// and this module still supports 8.2, where ordering by it is an unknown column.
if (self::isFeatureValuePositionSupported()
&& (bool) Configuration::get('PS_LAYERED_FILTER_FEATURE_VALUES_USE_POSITION')
) {
$order = 'ORDER BY v.`position` ASC';
} else {
$order = 'ORDER BY vl.`value` ASC';
}
$this->loadFeatureValues([$idFeature], $idLang);
}

$tempFeatureValues = $this->database->executeS(
'SELECT v.*, vl.*, ' .
'IF(lifvlv.`url_name` IS NULL OR lifvlv.`url_name` = "", NULL, lifvlv.`url_name`) AS url_name, ' .
'IF(lifvlv.`meta_title` IS NULL OR lifvlv.`meta_title` = "", NULL, lifvlv.`meta_title`) AS meta_title ' .
'FROM `' . _DB_PREFIX_ . 'feature_value` v ' .
'LEFT JOIN `' . _DB_PREFIX_ . 'feature_value_lang` vl ' .
'ON (v.`id_feature_value` = vl.`id_feature_value` AND vl.`id_lang` = ' . (int) $idLang . ') ' .
'LEFT JOIN `' . _DB_PREFIX_ . 'layered_indexable_feature_value_lang_value` lifvlv ' .
'ON (v.`id_feature_value` = lifvlv.`id_feature_value` AND lifvlv.`id_lang` = ' . (int) $idLang . ') ' .
'WHERE v.`id_feature` = ' . (int) $idFeature . ' ' .
$order
);
return $this->featureValues[$idLang][$idFeature];
}

foreach ($tempFeatureValues as $feature) {
$this->featureValues[$idLang][$idFeature][$feature['id_feature_value']] = $feature;
/**
* Loads in one query the values of the given features that are not loaded yet.
*
* @param int[] $idFeatures
* @param int $idLang
*/
public function loadFeatureValues(array $idFeatures, $idLang)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Unnecessary abstraction, why?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

it removes an N+1: getFeaturesBlock() ran one feature value query per displayed feature. Measured on 42 features, cold page: 42 queries / ~9.5 ms of SQL → 1 query / ~2.3 ms. getFeatureValues() keeps its signature for the other callers (Converter) and only loads what isn't loaded yet.

{
$idsToLoad = [];
foreach ($idFeatures as $idFeature) {
if (!isset($this->featureValues[$idLang][$idFeature])) {
$this->featureValues[$idLang][$idFeature] = [];
$idsToLoad[] = (int) $idFeature;
}
}
if (empty($idsToLoad)) {
return;
}

return $this->featureValues[$idLang][$idFeature];
// feature_value.position only exists from PrestaShop 9.0 (PrestaShop/PrestaShop#37042),
// and this module still supports 8.2, where ordering by it is an unknown column.
if (self::isFeatureValuePositionSupported()
&& (bool) Configuration::get('PS_LAYERED_FILTER_FEATURE_VALUES_USE_POSITION')
) {
$order = 'ORDER BY v.`position` ASC';
} else {
$order = 'ORDER BY vl.`value` ASC';
}

$tempFeatureValues = $this->database->executeS(
'SELECT v.*, vl.*, ' .
'IF(lifvlv.`url_name` IS NULL OR lifvlv.`url_name` = "", NULL, lifvlv.`url_name`) AS url_name, ' .
'IF(lifvlv.`meta_title` IS NULL OR lifvlv.`meta_title` = "", NULL, lifvlv.`meta_title`) AS meta_title ' .
'FROM `' . _DB_PREFIX_ . 'feature_value` v ' .
'LEFT JOIN `' . _DB_PREFIX_ . 'feature_value_lang` vl ' .
'ON (v.`id_feature_value` = vl.`id_feature_value` AND vl.`id_lang` = ' . (int) $idLang . ') ' .
'LEFT JOIN `' . _DB_PREFIX_ . 'layered_indexable_feature_value_lang_value` lifvlv ' .
'ON (v.`id_feature_value` = lifvlv.`id_feature_value` AND lifvlv.`id_lang` = ' . (int) $idLang . ') ' .
'WHERE v.`id_feature` IN (' . implode(',', $idsToLoad) . ') ' .
$order
);

foreach ($tempFeatureValues as $feature) {
$this->featureValues[$idLang][$feature['id_feature']][$feature['id_feature_value']] = $feature;
}
}
}
1 change: 1 addition & 0 deletions src/Product/Search.php
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,7 @@ private function addControllerSpecificFilters()
// we add this one specific category ID, otherwise, we will add everything using nleft and nright
if (Configuration::get('PS_LAYERED_FULL_TREE')) {
$this->getSearchAdapter()->addFilter('nleft', [$category->nleft], '>=');
$this->getSearchAdapter()->addFilter('nleft', [$category->nright], '<=');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hello guys, just one question, what this line does?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Logically this changes nothing, in the nested set, every subcategory already has its nleft between the category's nleft and nright. The extra condition is only there for MySQL. Without an upper bound on nleft, the index range runs to the end of the tree, so MySQL was starting from all of the shop's products instead of just the subtree.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Hlavtox is this PR ok for you ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, now I understand.

@LameuleFR Can you add some comment for next generations? :D Something like:

// Explicitly bound nleft on both sides to limit the index scan
 // to the current category subtree. The upper bound is logically
 // redundant but helps MySQL use a much smaller index range.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done in 988e00d, on both places using this bound (Search.php and the categories block in Block.php).

$this->getSearchAdapter()->addFilter('nright', [$category->nright], '<=');
} else {
$this->addFilter('id_category', [$idCategory]);
Expand Down
Loading
Loading