1

I am working a asp .net core web API with EF core. I wrote this query. But this take 20-30seconds to execute.

Anyone have idea to improve this query.

var hotels = await _context.Hotels
                    .Where(i => (i.DestinationCode == request.Destination))
                    .Select(i => new HotelListHotelVm
                    {
                        Item1 = i.Item1,
                        Item2 = i.Item2,
                        Item3 = i.Item3,
                        
                        Item4Code = i.Item4Code,
                        Item4Description = i.Item4.TypeDescription,
                        Item5 = i.Item5.Select(x => new HotelListHotelVm.HotelListItem5Vm
                        {
                            Code = x.Item5Code,
                            Description = x.Item5.Description,
                        }).Where(x =>(incomingItem5s.Length > 0 ) ? (incomingItem5s.Contains(x.Code)) : (x.Code != "")),
                        Item6 = i.Item6.Select(x => new HotelListHotelVm.HotelListHotelItem6Vm
                        {
                            Id = x.Id,
                            Item6TypeCode = x.Item6TypeCode,
                            Order = x.Order,
                            Path = x.Path,
                            VisualOrder = x.VisualOrder,
                        }).Take(3),
                        HotelFacilities =  i.Facilities.ToList().Distinct().Take(6).Select(x => new HotelListHotelVm.HotelListFacilityVm {
                            Id = x.Id,
                            FacilityGroupCode = x.FacilityGroupCode,
                            HotelFacilityGroupDescription = x.FacilityGroup.Description,
                            FacilityCode = x.FacilityCode
                        }),
                    })
                    .Where( i => ((incomingItem4.Length > 0 ) ? (incomingItem4.Contains(i.Item4Code)) : (i.Item4Code != ""))   )
                    .OrderByDescending(i => i.Code)
                    .PaginatedListAsync(request.PageNumber, request.PageSize);



                    foreach( var item in hotels.Items){
                        foreach(var facility in item.HotelFacilities){
                            foreach( var fac in  _context.Facilities){
                        
                                if(facility.FacilityCode == fac.Code){
                                    facility.HotelFacilityDescription = fac.Description;
                                }
                            }
                        }
                    }

I f I remove those foreach code, The query takes 8-10s to execute. But I need those foreach codes. Because I need the HotelFacilityDescription

Any suggestion for optimize the query ?

Edit The i.Facilities - model

public class HotelFacility 
    {
        // removed some
        public int FacilityCode { get; set; }

        public int FacilityGroupCode { get; set; }
        public FacilityGroup FacilityGroup { get; set; }

        public int HotelCode { get; set; }
        public Hotel Hotel { get; set; }
    }
}
6
  • unfortunately we are using version 5.0 Commented Dec 9, 2021 at 16:20
  • (1) Please include the model class pointed by i.Facilities collection navigation property in the question. (2) Also explain what are you trying to achieve with .ToList().Distinct(), as it doesn't do anything useful - the Id is already unique, so distinct operator does not change the resulting set, but might affect the query performance in the database query optimizer is not smart enough to ignore it. Commented Dec 10, 2021 at 8:34
  • (1) I have updated. (2) Because There is some duplicate FacilityCode Commented Dec 10, 2021 at 8:59
  • (2) You removed some properties from the model which might be essential. The query shows that there must be a property called Id, which most probably is PK, i.e. unique, so distinct has no effect. Distinct is using all properties, not just FacilityCode. That's why I asked what are you trying to achieve - according to your comment it definitely does not do what you are expecting. Commented Dec 10, 2021 at 10:54
  • (1) Also , don't you have public Facility Facility { get; set; } navigation property (like you have for FacilityGroup)? If no, why not? Make sure you have it, and then simply use it inside LINQ to Entities query, e.g. FacilityCode = x.FacilityCode, HotelFacilityDescription = x.Facility.Description and remove the loop. Commented Dec 10, 2021 at 11:14

3 Answers 3

2

_context.Facilities will be enumerated (i.e. database will be called) for every iteration of previous loops. The quick fix is to call it ones and store results in variable:

var facilities = _context.Facilities.ToList();
foreach( var item in hotels.Items){
    foreach(var facility in item.HotelFacilities){
        foreach(var fac in facilities){
                        
            if(facility.FacilityCode == fac.Code){
                facility.HotelFacilityDescription = fac.Description;
            }
        }
    }
}

Next improvement can be converting facilities into Dictionary for searching purposes.

Even better approach can be writing query joining with _context.Facilities on database side (but here more info needed).

Sign up to request clarification or add additional context in comments.

3 Comments

Thank you @Guru. Is there any suggestion to improve this query var hotels = await _context.Hotels........ Because If I comment the foreach codes, The rest of query also take 2s-8s.
@hanushic It is hard to say. Check the generated SQL, check the execution plan. Maybe some indexes will help. Or changing the pagination since .OrderByDescending(i => i.Code) with skip-take can be very costly.
Okay I will check. Thanks again bro
1

I've read this a couple times, but it looks like the relationship for Hotel.Facilities is a Facility, so could you not just do:

  HotelFacilities =  i.Facilities.ToList().Distinct().Take(6).Select(x => new HotelListHotelVm.HotelListFacilityVm {
                        Id = x.Id,
                        FacilityGroupCode = x.FacilityGroupCode,
                        HotelFacilityGroupDescription = x.FacilityGroup.Description,
                        FacilityCode = x.FacilityCode,
                        HotelFacilityDescription = x.Description
                    }),

If for some reason Hotel.Facilities is not pointing at a Facility, but is a Many-to-Many HotelFacilityGroup entity to a FacilityGroup, that also contains a FacilityCode, if the associated FacilityGroup has access to a set of Facilities beneath it you could leverage that:

Edit: It sounds like multiple Facilities share the same Code where some may have a null description. Provided that the facilities matching the code would be within the same facility group and not consider the same Code within different facility groups. If you need to match the code across all facilities then there probably isn't much of an alternative to loading the entire set of facility codes & descriptions.

  HotelFacilities =  i.Facilities.ToList().Distinct().Take(6).Select(x => new HotelListHotelVm.HotelListFacilityVm {
                        Id = x.Id,
                        FacilityGroupCode = x.FacilityGroupCode,
                        HotelFacilityGroupDescription = x.FacilityGroup.Description,
                        FacilityCode = x.FacilityCode,
                        HotelFacilityDescription = x.FacilityGroup.Facilities.Where(f => f.Code == x.FacilityCode && f.Description != null).Select(f => f.Description).FirstOrDefault()
                    }),

That would avoid the need to go load all of the facilities to resolve that code. Otherwise, if you do need to fetch across all facilities, pre-loading them would be the way to go, but rather than fetching the entire Facility entity I would recommend just the values you need, the Code and the Description. This cuts down on the amount of memory needed and potentially be a faster query:

var facilities = _context.Facilities
    .Select(f => new 
    {
        f.Code,
        f.Description
    }).ToList();

Edit: From there, finding a match using:

foreach( var facility in hotels.Items.SelectMany(x => x.HotelFacilities)
{
    facility.HotelFaciltyDescription = facilities
        .Where(x => x.Code == facility.FacilityCode 
            && !string.IsNullOrEmpty(x.Description)
        .Select(x => x.Description)
        .FirstOrDefault();
}

I would recommend an OrderBy clause to ensure the selection of the facility is predictable as it sounds like there could be multiple matches on a code with a non-null description.

3 Comments

Yes. FaciltyGroup has a relationship with Facility. But some description are null. So I used Last one. Thanks you. But still It takes 8seconds.
Can I use, like this ` var facilitiesList = _context.Facilities.ToList(); ` HotelFacilities = hotelFacilitiesList.Where(x => (x.HotelCode == i.Code)) .Select(x => new HotelListHotelVm.HotelListFacilityVm { Id = x.Id, FacilityGroupCode = x.FacilityGroupCode, HotelFacilityGroupDescription = x.FacilityGroup.Description, FacilityCode = x.FacilityCode })
I made a couple edits to the examples. Overall those queries will be expensive with things like Distinct calls or querying across entire tables to find loosely coupled or unreliable values.
0

The loop can be eliminated by projecting the value in the LINQ to Entities query.

It would have been quite easy if you had relationship and navigation property like other *Code fields. But as clarified in the comments, there is no such relationship, so you have to resort to old good manual left other join to emulate what navigation property provide automatically, e.g.

HotelFacilities = i.Facilities.ToList().Distinct().Take(6)
    // left outer join with Facilities
    .SelectMany(x => _context.Facilities
        .Where(f => x.FacilityCode == f.Code).DefaultIfEmpty(),
    (x, x_Facility) => new HotelListHotelVm.HotelListFacilityVm
    {
        Id = x.Id,
        FacilityGroupCode = x.FacilityGroupCode,
        HotelFacilityGroupDescription = x.FacilityGroup.Description,
        FacilityCode = x.FacilityCode,
        HotelFacilityDescription = x_Facility.Description // <--
    }),

Here x_Facility emulates optional reference navigation property x.Facility if existed.

In case you need just single property from the related table, instead of a left join you could also use the original query with single value returning correlated subquery inside the projection, e.g.

HotelFacilities = i.Facilities.ToList().Distinct().Take(6)
    .Select(x => new HotelListHotelVm.HotelListFacilityVm
    {
        Id = x.Id,
        FacilityGroupCode = x.FacilityGroupCode,
        HotelFacilityGroupDescription = x.FacilityGroup.Description,
        FacilityCode = x.FacilityCode,
        HotelFacilityDescription = _context.Facilities
            .Where(f => x.FacilityCode == f.Code)
            .Select(f => f.Description)
            .FirstOrDefault() // <--
    }),

or even

HotelFacilityDescription = _context.Facilities
    .FirstOrDefault(f => x.FacilityCode == f.Code).Description

All these will eliminate the need of the post loop executiong additional database queries. You can test them and take the one with best performance (#2 and #3 produce one and the same SQL, so it's a matter of taste - the choice is between #1 and #2/3).

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.