Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Product relations for categories #353

Merged
merged 8 commits into from
Oct 11, 2023

Conversation

Jade-GG
Copy link
Collaborator

@Jade-GG Jade-GG commented Oct 6, 2023

As it turns out, we can use the catalog_category_product_index_storeX tables which contains the visibilities of the products. This gives a ~4x speedup in the query if you just want a products count, as joining the whole flat table is pretty slow™. To make this easy I've just added a productIndices relation along with the products index to use in your withCount.

)
->withoutGlobalScopes()
->whereIn((new(config('rapidez.models.category_product')))->qualifyColumn('visibility'), [2, 4]);
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This isn't very DRY, is there maybe a nice way to combine productIndices into this relation?

@royduin
Copy link
Member

royduin commented Oct 10, 2023

Maybe belongsToMany and add the columns with withPivot? That way you just have 1 relation as productIndices is a bit of a weird relation.

@Jade-GG
Copy link
Collaborator Author

Jade-GG commented Oct 10, 2023

That way you just have 1 relation as productIndices is a bit of a weird relation.

productIndices isn't necessarily needed, it's there just for efficiency :)

From what I understand using withPivot would only add the visibility information (i.e. the extra information that's inside the pivot table) to the query, which we also already have for the products, so I'm not sure how it'd help here as it wouldn't improve efficiency in any way. I tried looking for an onlyPivot() function or similar but I couldn't find anything.

@royduin
Copy link
Member

royduin commented Oct 10, 2023

Mm, maybe a rename, productRelations, productLinks or productPivot? @indykoning thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

CategoryProduct may conflict in name as there, i personally tend to stick close to the original table name like seen here https://github.com/Genaker/laragento/tree/main/src/Models

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's tough because then we also should rename the Product and Category models to CatalogProduct and CatalogCategory. With how it's set up right now, I feel like this one specifically is more consistent with the others and if this is something we want to uphold in the future we should probably change this in a dedicated PR so it shows up in the changelog.

src/Commands/IndexCategoriesCommand.php Outdated Show resolved Hide resolved
@Jade-GG
Copy link
Collaborator Author

Jade-GG commented Oct 11, 2023

I've removed productIndices for now as the has() version of the query is much more efficient regardless of whether or not the products table is joined.

src/Models/Category.php Outdated Show resolved Hide resolved
@royduin royduin merged commit eeae34b into rapidez:master Oct 11, 2023
6 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants