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

fix(pat-5034): c# - include order-by when computing group-by #210

Merged
merged 2 commits into from
Dec 19, 2023

Conversation

ajmasci
Copy link
Contributor

@ajmasci ajmasci commented Dec 19, 2023

  • Include order-by expressions when computing group-by clause.

Test plan

  • Added unit tests.

Copy link

graphite-app bot commented Dec 19, 2023

Your org has enabled the Graphite merge queue for merging into main

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

You can enable merging using labels in your Graphite merge queue settings.

}
}

if (expandedSelection.Where((x) => x.GetType().Name == typeof(AggregateFieldExpr<>).Name).Any())
Copy link
Contributor

Choose a reason for hiding this comment

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

Huh, the RHS of this expression is interesting:

x.GetType().Name == typeof(AggregateFieldExpr<>).Name

Can you explain this a little bit? I'm not sure how to read it. It almost looks like the expression as a whole is acting as an existential qualifier ("there exists some type of AggregateFieldExpr whose name is equal to x.GetType().Name").

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this is the equivalent of the x instanceof AggregateFieldExpr.
I recall having a bit of a struggle to figure this out. Not sure if there's a more direct way to do this in C# for a template type.

static/csharp/Dpm/DpmAgentQueryFactory.cs Show resolved Hide resolved
@ajmasci ajmasci force-pushed the ajith/pat-5034-python-order-by-in-group-by branch from c0b4f4b to 7ade3e2 Compare December 19, 2023 20:28
@ajmasci ajmasci force-pushed the ajith/pat-5034-csharp-order-by-in-group-by branch from 71cbc59 to c81645a Compare December 19, 2023 20:29
@ajmasci ajmasci force-pushed the ajith/pat-5034-python-order-by-in-group-by branch from 7ade3e2 to 4d7140e Compare December 19, 2023 20:59
@ajmasci ajmasci force-pushed the ajith/pat-5034-csharp-order-by-in-group-by branch from c81645a to b31e7ba Compare December 19, 2023 20:59
Copy link

graphite-app bot commented Dec 19, 2023

Merge activity

@ajmasci ajmasci changed the base branch from ajith/pat-5034-python-order-by-in-group-by to main December 19, 2023 22:34
Copy link

graphite-app bot commented Dec 19, 2023

Merge activity

@graphite-app graphite-app bot merged commit b31e7ba into main Dec 19, 2023
10 checks passed
@graphite-app graphite-app bot deleted the ajith/pat-5034-csharp-order-by-in-group-by branch December 19, 2023 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants