Skip to content

Conversation

@chenkovsky
Copy link
Contributor

Which issue does this PR close?

Rationale for this change

currently, distinct on is supported for aggregation.

What changes are included in this PR?

support distinct on for aggregation.

Are these changes tested?

UT

Are there any user-facing changes?

No

@github-actions github-actions bot added sql SQL Planner sqllogictest SQL Logic Tests (.slt) labels Nov 11, 2025
@Jefffrey
Copy link
Contributor

Could you please provide a more detailed PR body? The template is there for a reason, to make it easier for reviewers to understand the intended change. Right now I can't gleam any meaning at all from the body; in fact it just leaves me confused:

Rationale:
currently, distinct on is supported for aggregation.
Changes in this PR:
support distinct on for aggregation.

This doesn't make any sense to me.

Also would it close #17256?

@alamb
Copy link
Contributor

alamb commented Nov 11, 2025

Marking as draft

I second @Jefffrey 's commentary. @chenkovsky your contributions are appreciated -- but they would be more appreciated (and more likely to get reviews faster / more usefully) if you made them easier to review

As @Jefffrey says, one part of making the code easier to review is a good description so reviewers don't have to research the background for the proposal

Other things that can help:

  1. Breaking larger PRs into smaller ones (not relevant for this PR)
  2. Using inline comments to draw our attention to important parts of the PR that might not be obvious from the body
  3. Ensuring the code changes are well commented and tested (specifically I often look at the tests first so I know what the code is doing, so if it is not clear from the tests what the intent of a PR is, that will make it harder to review)

@chenkovsky chenkovsky closed this Nov 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

sql SQL Planner sqllogictest SQL Logic Tests (.slt)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants