Skip to content

fix groupconcat#24941

Open
daviszhen wants to merge 10 commits into
matrixorigin:mainfrom
daviszhen:0611-fix-groupconcat
Open

fix groupconcat#24941
daviszhen wants to merge 10 commits into
matrixorigin:mainfrom
daviszhen:0611-fix-groupconcat

Conversation

@daviszhen

Copy link
Copy Markdown
Contributor

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #24359

What this PR does / why we need it:

修改 group_concat 的order by 改写.

@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

return 0, nil
}

func compareGroupConcatOrderValue(left, right any) int {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am very surprised we need such a function. This is generic order by clause and should reuse all the code from say, sort operator.

More: type is ALWAYS available, so we should NOT need a type switch.

@XuPeng-SH XuPeng-SH left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Requesting changes for two semantic regressions in the new GROUP_CONCAT ... ORDER BY rewrite.

  1. DISTINCT + ORDER BY can keep the wrong representative row

    • In concat2.go, DISTINCT dedup happens first via distinctHash.fill(...) on only the concat arguments.
    • The ORDER BY payload is sorted later in flushGroup().
    • So for cases like GROUP_CONCAT(DISTINCT x ORDER BY y), duplicate x values with different y values now keep whichever row was seen first, not the one implied by the ORDER BY.
  2. Explicit NULLS FIRST / NULLS LAST is dropped

    • bindGroupConcatOrderBy() in having_binder.go only records ASC/DESC into orderFlags.
    • Execution then compares nils with the default behavior in types.CompareValue, so explicit null placement is no longer preserved.

I think both need to be fixed or rejected explicitly before approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working size/XL Denotes a PR that changes [1000, 1999] lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants