My requirement is to get max amount of opportunity on a custom field(Highest_Amount__c) on account object. i need to write trigger for this

Question

trigger opportunityAccountTrigger on Opportunity (after insert) {
   
    set<id> accIds = new set<id>();
    for(Opportunity opp : trigger.new){
         accIds.add(opp.AccountId);
    }
    list<Account> acc = new List<Account>();
    For(AggregateResult ar :[select id, Account.Highest_Amount__c,max(Amount) maxAmt 
                              from opportunity where id =:accIds group by id,Account.Highest_Amount__c]){
                            Account accs = new Account(id= (id)ar.get('expr0'),Highest_Amount__c=(double)ar.get('maxAmt'));
                              acc.add(accs)  ;  
        
    }
    update acc;
    
    
}

I write above trigger but when i create any new opportunity, i am getting below error

Answer 1

First and foremost

This is something you should do via a rollup summary field. Don't try to re-invent the wheel.

The issue

Your query has one main problem.
You're trying to include Account.Highest_Amount__c in the SELECT clause of your query, and also group by it.

This is incorrect. At the very minimum, you need to remove this related field from the SELECT and GROUP BY clauses.

When you use GROUP BY, then every field included in the SELECT clause either needs to be used in an aggregate function like MAX(), COUNT(), AVG(), etc., or it needs to appear in the GROUP BY clause.

Why though?

Because while you can GROUP BY related fields (e.g. SELECT Account.Name, MAX(Amount) FROM Opportunity GROUP BY Account.Name), not all fields are "groupable". In your case, for one reason or another, Highest_Amount__c is not groupable.

Even if you could group by Account.Highest_Amount__c though, it would not be what you want to do.
You're trying to find the max revenue (for an Opp) per Account. The current value of Highest_Amount__c is irrelevant. You don't care so much about what the current highest amount is, just what the new highest amount is.

Your only concern is to get the new max value of Amount on Opps related to a given Account, and GROUP BY AccountId accomplishes that all on its own.

Again, don't re-invent the wheel

A rollup summary field is the right approach here. Salesforce has already worked out what needs to happen, so you should use what they know instead of trying to recreate it yourself.

If you try to modify your code to get through the error, I guarantee that you will still make subtle mistakes. The prime example here is that you are not considering what happens if an Opportunity is deleted (you also aren't considering the case where an Opportunity is updated).

Even if your code didn't run into an error and you were operating on all of the appropriate trigger contexts, it would produce an incorrect result if an Account were to lose its last Opportunity.

I don't think you're at an appropriate level yet to code this yourself (or test it, for that matter). Just go with the rollup summary field. For things that you can't make a rollup summary field for, the Declarative Lookup Rollup Summary tool is the next best option.

A custom-written trigger should be the last resort here because you need to have a firm grasp on what it is that you actually need to do (and keep in mind all of the edge cases that you can run into), and you need to be able to write tests for it as well (which is generally accepted as being harder to do than writing the code in the first place).

If you can't explain precisely why you need to write this yourself¹, then using existing functionality is the correct approach.

^{1: "because that's what I was told to do" is not sufficient. What I'm talking about here is more along the lines of "our org is using too many queries, and we need to fit this into the trigger framework that we're using". You must have that level of detail and need to justify doing this in code you write yourself.}