fix: Postrgres group aggregation

[srameshr]

No description provided.

[srameshr]

@dplewis @cbaker6
Here is a new PR

[dplewis]

@srameshr Can you add tests?

[srameshr]

@dplewis There are already tests for group by, limit, sort, where. My changes do not affect the existing tests. Also all our local code works and returns the right values. So, I assume we do not need tests for this one.

[dplewis]

Can you explain the problem you are trying to fix? Just so we can determine if there are side-effects.

[srameshr]

@dplewis
The queries used to fail when * was used with GROUPBY operators and the order of few operators where off when GROUPBY was present.

[srameshr]

@dplewis
Can you please merge this? All the existing test cases that was there to handle groupBy and other conditions are working as it sits.

[srameshr]

@dplewis Anything on this?

[dplewis]

@srameshr Sorry for the late reply. Can you resolve the conflict so that we can get this into the next release?

[cbaker6]

@srameshr Sorry for the late reply. Can you resolve the conflict so that we can get this into the next release?

My guess is it's conflicting with the code I added in #6506 because we were editing the same method/lines (note, I'm not able to see the conflict, so I'm guessing).

@srameshr if this is the case, then the conflict is somewhere around this line:

const originalQuery = `SELECT ${columns.join()} FROM $1:name ${wherePattern} ${sortPattern} ${limitPattern} ${skipPattern} ${groupPattern}`;
const qs = explain ? this.createExplainableQuery(originalQuery) : originalQuery;

You code:

if (groupPattern) {
      columns.forEach((e, i, a) => {
        if (e && e.trim() === '*') {
          a[i] = '';
        }
      });
    }

Should be added before the line I mentioned above and that should resolve the conflict (if that's really the conflict) with everything still working. Let me know if that helps.

[srameshr]

@cbaker6 Are your changes in master?

[srameshr]

@dplewis @cbaker6
I have fixed the conflicts and updated the code. Can you please look into this? We have a release o n Monday and waiting on it.

[cbaker6]

@dplewis @cbaker6
I have fixed the conflicts and updated the code. Can you please look into this? We have a release o n Monday and waiting on it.

I think you forgot to lint

[srameshr]

@cbaker6 I did lint. Its a pre commit hook that automatically runs

[codecov]

Codecov Report

Merging #6522 into master will decrease coverage by 0.00%.
The diff coverage is 96.77%.

@@            Coverage Diff             @@
##           master    #6522      +/-   ##
==========================================
- Coverage   93.91%   93.90%   -0.01%     
==========================================
  Files         169      169              
  Lines       11963    11968       +5     
==========================================
+ Hits        11235    11239       +4     
- Misses        728      729       +1     

Impacted Files	Coverage Δ
...dapters/Storage/Postgres/PostgresStorageAdapter.js	96.33% <96.77%> (-0.15%)	⬇️
src/RestWrite.js	93.81% <0.00%> (+0.16%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b085717...abd4147. Read the comment docs.

[srameshr]

@cbaker6
I did run npm run lint again it is just messing up this line
https://github.com/parse-community/parse-server/pull/6522/files#diff-b0af2ee2c26859a8aa6d8d0a0cf3a64aR2587

[cbaker6]

@cbaker6
I did run npm run lint again it is just messing up this line
https://github.com/parse-community/parse-server/pull/6522/files#diff-b0af2ee2c26859a8aa6d8d0a0cf3a64aR2587

Are you doing npm run lint locally before you push your code? It's easier to handle the problem locally before pushing. Whenever my lint passes locally, I don't have an issue on CI

[srameshr]

@cbaker6
Yes, running it locally and it passes fine. But when I run commit, the line I have mentioned throws a lint error every time. Regardless of how I change it, so I just did a --no-verify while commit and left it to be merged.

[srameshr]

@dplewis @acinader Can I expect a new release this week with the above changes going in?

[srameshr]

@dplewis Any timeline as to when the next release will be? Could you do it this week?

[srameshr]

@dplewis @acinader Are you guys planning a release this week? Is there a way we can create a release? I am waiting for this because our AWS instances do not allow npm install to run from github repo links for some reason.