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

分库分表:聚合函数含groupby字句 #193

Merged
merged 4 commits into from
Apr 24, 2023

Conversation

juniaoshaonian
Copy link
Collaborator

No description provided.

internal/merger/groupbyMerger/aggregatorMerger.go Outdated Show resolved Hide resolved
internal/merger/groupbyMerger/aggregatorMerger.go Outdated Show resolved Hide resolved
//go:linkname convertAssign database/sql.convertAssign
func convertAssign(dest, src any) error

type GroupByColumn struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

能不能在 merger 里面定义一个统一的 Column ? 我记得在很多地方都使用到了类似的结构。不过要是差异很大就没有必要抽取出来。

我主要是顾虑到,如果每一个实现都有类似的东西,但是都是各自定义一份,用户用起来是很困惑的,一会这个 Column,一会那个 Column

Copy link
Contributor

Choose a reason for hiding this comment

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

又或者可以考虑放过去 aggregator 包?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

可以我把这两个抽到merger的type中你感觉还行不

Copy link
Contributor

Choose a reason for hiding this comment

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

感觉可以

internal/merger/groupbyMerger/aggregatorMerger.go Outdated Show resolved Hide resolved
internal/merger/groupbyMerger/aggregatorMerger.go Outdated Show resolved Hide resolved
internal/merger/groupbyMerger/aggregatorMerger.go Outdated Show resolved Hide resolved
internal/merger/groupbyMerger/aggregatorMerger_test.go Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 20, 2023

Codecov Report

Merging #193 (55cd43c) into dev (c79a850) will decrease coverage by 0.31%.
The diff coverage is 82.02%.

@@            Coverage Diff             @@
##              dev     #193      +/-   ##
==========================================
- Coverage   86.16%   85.85%   -0.31%     
==========================================
  Files          43       44       +1     
  Lines        3087     3274     +187     
==========================================
+ Hits         2660     2811     +151     
- Misses        345      372      +27     
- Partials       82       91       +9     
Impacted Files Coverage Δ
internal/merger/utils/scan.go 60.86% <60.86%> (ø)
...nternal/merger/groupby_merger/aggregator_merger.go 83.60% <83.60%> (ø)
internal/merger/aggregatemerger/aggregator/avg.go 100.00% <100.00%> (ø)
...nternal/merger/aggregatemerger/aggregator/count.go 100.00% <100.00%> (ø)
internal/merger/aggregatemerger/aggregator/max.go 100.00% <100.00%> (ø)
internal/merger/aggregatemerger/aggregator/min.go 100.00% <100.00%> (ø)
internal/merger/aggregatemerger/aggregator/sum.go 100.00% <100.00%> (ø)
internal/merger/aggregatemerger/merger.go 93.52% <100.00%> (+1.36%) ⬆️
internal/merger/sortmerger/merger.go 89.90% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Contributor

@flycash flycash left a comment

Choose a reason for hiding this comment

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

internal/merger/utils/convert_Assign.go Outdated Show resolved Hide resolved
internal/merger/utils/scan.go Show resolved Hide resolved
@flycash flycash merged commit 400c80e into ecodeclub:dev Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

分库分表:结果集处理——聚合函数(含 Group By 子句)
2 participants