VirtoCommerce/vc-module-catalog

Query optimization based om empty collections

yecli opened this issue · 6 comments

yecli commented

Need to optimize GetByIds methods by skipping redundant requests in case of empty collections like in Cart module.

Places where need to do optimization:
GetCatalogsByIds
GetItemByIds
GetPropertiesByIds
GetAllCatalogProperties
GetPropertyDictionaryItemsByIds

As proposed by @asvishnyakov and @artem-dudarev, we could add IQueryable<> extension method to platform that implements this logic and use it in this cases.

I would like to have extension method and use method chain instead of if-wrapping, i.e.
result = Cart.NotNull.Where(...).ToArray();

yecli commented

Not clear how to do this, as our goal here is not just to ensure that IEnumerable<> is not null or empty, but to skip queries execution in that case. Could execution of query be skipped in extension method?

I do it this way:

public class MyRepository
{
        public virtual IList<CarouselBannerEntity> GetCarouselBannersByIds(IList<string> ids)
        {
            return CarouselBanners
                .Where(x => ids.Contains(x.Id))
                .LoadWithRelatedEntities(ids, LoadCarouselBannerRelatedEntities);
        }

        public virtual IList<CarouselBannerEntity> GetCarouselBannersByObjectIds(IList<string> objectIds, string objectType)
        {
            return CarouselBanners
                .Where(x => x.ObjectType == objectType && objectIds.Contains(x.ObjectId))
                .LoadWithRelatedEntities(objectIds, LoadCarouselBannerRelatedEntities);
        }

        protected virtual void LoadCarouselBannerRelatedEntities(IList<string> ids)
        {
            CarouselBannerStores.Where(x => ids.Contains(x.CarouselBannerId)).Load();
            CarouselBannerUserGroups.Where(x => ids.Contains(x.CarouselBannerId)).Load();
        }
}
public static class QueryableExtensions
{
    public static IList<T> LoadWithRelatedEntities<T>(this IQueryable<T> query, IEnumerable<string> keys, Action<IList<string>> loadRelatedEntitiesAction)
        where T : Entity
    {
        IList<T> result;

        if (keys?.Any() == true)
        {
            result = query.ToList();

            if (result.Any())
            {
                var ids = result.Select(x => x.Id).ToList();
                loadRelatedEntitiesAction(ids);
            }
        }
        else
        {
            result = new T[0];
        }

        return result;
    }
}

pretty nice! but Array.Empty() instead of T[0] looks better for me, last one may confuse (access to element with zero index)

yecli commented

Yes, wanted to change that too.

yecli commented

After discussion with @tatarincev decided to not use extension method and handle references loading in place.