分库分表：merger 排序实现 #166

juniaoshaonian · 2023-03-04T04:11:27Z

scan方法我没有实现

codecov-commenter · 2023-03-04T04:12:49Z

Codecov Report

Merging #166 (085981b) into dev (277a881) will increase coverage by 0.13%.
The diff coverage is 81.33%.

❗ Current head 085981b differs from pull request most recent head a4e3c3c. Consider uploading reports for the commit a4e3c3c to get more accurate results

@@            Coverage Diff             @@
##              dev     #166      +/-   ##
==========================================
+ Coverage   79.69%   79.82%   +0.13%     
==========================================
  Files          30       32       +2     
  Lines        2354     2563     +209     
==========================================
+ Hits         1876     2046     +170     
- Misses        394      423      +29     
- Partials       84       94      +10

Impacted Files	Coverage Δ
internal/merger/sortmerger/merger.go	`78.28% <78.28%> (ø)`
internal/merger/sortmerger/heap.go	`96.96% <96.96%> (ø)`
internal/merger/batchmerger/merger.go	`73.33% <100.00%> (+0.45%)`	⬆️

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

flycash · 2023-03-04T12:43:47Z

这个思路是没问题，不过有几个点：

所有的 merger 不能依赖于 ORM 的任何其它包，因为将来我就要拆出去的，给别的分库分表用的；
要采用延迟读数据的策略。你可以预期用户传入的 results []*sql.Rows 中的每一个元素的数据，都是拍好序了的，所以对你来说，你就是一个延迟归并的过程。比如说 A B C 三个，当用户第一次调用 Next，你需要取出来 A B C 的第一条数据，然后找到最小的那个，把指针指向这个最小的，假如说是 A 的，那么 B C 的你就要临时存起来。等调用 Scan ，那么你就是用刚才 A 中的数据去填充，这里面也就是我说的类型转换很麻烦，因为之前你在 A 读出来的时候，可以考虑用 []byte 去接收，然后在 Scan 的时候就要转过去用户需要的数据类型；

数据类型的转换，在 sql 包里面是 convertAssignRows 这个东西。所以基本上就是抄一遍这个方法。实在无力吐槽这个关键的方法它居然是私有的。

这里面是有隐患的，就是你在 Next 的时候就需要取出来数据，然后临时存着。理论上来说用 []byte 不会有什么问题，但是我觉得这里面可能有不少的坑。

flycash · 2023-03-04T12:44:57Z

你考虑这样一个场景，就是分页的场景，比如说排序之后取（LIMIT 100,10），那么你完全不需要全部排序一遍，而是就只排序一部分。

flycash · 2023-03-04T12:47:13Z

那么 Scan 里面，就调用你抄过来的那个 ConvertAndAssign 就可以。

internal/merger/sortmerger/merger.go

flycash · 2023-03-06T02:43:49Z

internal/merger/sortmerger/merger.go

+	DESC Order = false
+)
+
+type Merger[T any] struct {


这个 Merger 肯定不可能是泛型的。注意，在 merger 这个包，它完全没有任何将数据转化为结构体的概念。

juniaoshaonian · 2023-03-07T08:53:03Z

迭代过程中的错误处理我是这样搞的，无论是排序过程中遇到的，还是sql.rows的Next方法遇到的错误。都会结束迭代，调用close方法然后返回false。

script/setup.sh

internal/merger/sortmerger/merger.go

internal/merger/sortmerger/merger_test.go

internal/merger/sortmerger/merger.go

internal/merger/sortmerger/merger_test.go

internal/merger/sortmerger/merger.go

internal/merger/sortmerger/merger_test.go

internal/merger/sortmerger/merger.go

internal/merger/sortmerger/merger_test.go

flycash

@longyue0521 @Stone-afk 麻烦看看。@Stone-afk 你会是主要用户，所以你看看是否符合你的预期

internal/merger/sortmerger/merger_test.go

longyue0521 · 2023-03-10T02:30:05Z

internal/merger/sortmerger/merger_test.go

+	}{
+
+		{
+			name: "排序列id ASC_多个sqlRows_每个sqlRows返回行数相同_交叉读取各个sqlRows",


你对交叉读的定义是什么? sqlRows[1~3]每个有两行数据,读取顺序为1-2->3, 1-2-3这样读你给它一个名字叫什么?

你可以不用我给的名字,你自己起名字但是要对应好场景并始终保持一致,你现在的“交叉读”的意义是不一致的.

请给出 “轮训”“顺序”“交叉”的实例, 假设有3个sqlRows,每个sqlRows返回2行数据.

1.1 1.2 2.1 2.2 3.1 3.2 叫什么?
1.1 2.1 3.1 1.2 2.2 3.2 叫什么?
1.1 2.1 2.2 3.1 1.2 3.2 叫什么?

明确定义后,自行校对一下测试用例是否符合你的定义.

就两种，顺序一个sql.rows读完再读另外一个直至全部读完，交叉就是一个读了部分再读其他

可以，顺序读定义明确；交叉要不要分为部分交叉和全交叉？为什么不要？

那我再加一个部分交叉的例子：有一行一次性全部读完。两行的交叉着读

longyue0521 · 2023-03-10T02:40:44Z

internal/merger/sortmerger/merger_test.go

+			},
+		},
+		{
+			name: "排序列id ASC_多个sqlRows_部分sqlRows返回行数相同_顺序读取各个sqlRows",


既然你不想重新设计这个测试用例(#166 (comment)) ,那就补充缺少的测试用例以覆盖 —— 各个sqlRows返回行数都不同的场景

我下面我记得写了一个呀

internal/merger/sortmerger/merger_test.go

longyue0521 · 2023-03-11T10:55:49Z

internal/merger/sortmerger/merger_test.go

+			},
+		},
+		{
+			name: "排序列id ASC_部分sqlRows返回行数相同_顺序读取各个sqlRows",


#166 (comment) 这个问题仍然没有解决，把你觉得写了的那个用例的行数发一下。

我觉得 longyue 的想法是说，在这个测试用例设计上，有几个因素会影响 Next 在不同的 sql.Rows 里面切换：

完全交叉、部分交叉和完全不交叉

行数，部分数量比较少的行会提前结束，那么其它 Rows 依旧能够正确被读取完毕。那么极端的情况就是大家行数都不同，行数都相同；再结合行数可以为0，那么可以是头几个为行数为0，后几个行数为0，中间几个行数为0

有一些你在这个测试用例里面可以不体现的，就是排序列是正序还是倒序，以及多少个排序列。这部分是你的 heap 实现来保证的，这边有一个简单的例子就差不多——主要是确保 Heap 和 Next 运作正常。

完全交叉，行数相同

完全交叉，行数部分不同

完全交叉，行数完全不同

部分交叉，行数相同

部分交叉, 行数部分不同

部分交叉，行数完全不同

顺序读，行数相同

顺序读，行数部分不同

顺序读，行数完全不同
// 这边就不分交叉读什么的了，上面已经讨论过了，以完全交叉读为例

返回的行数为0，在前面

返回的行数为0，在中间

返回的行数为0，在后面

返回的行数全部为0

排序列返回的顺序和数据库里的字段顺序不一致

@longyue0521 @flycash 这样搞你们觉得还行不

有一些你在这个测试用例里面可以不体现的，就是排序列是正序还是倒序，以及多少个排序列。这部分是你的 heap 实现来保证的，这边有一个简单的例子就差不多——主要是确保 Heap 和 Next 运作正常。

我赞同 @flycash的建议，但这是有前提的——你抽取出去的heap有对应的测试覆盖来保证。现在的情况是抽取出的heap没有对应用的测试测试，所以这里只能用类似于”集成测试“或粒度更大的单元测试的概念来保证正确性。

返回的行数为0，在前面
返回的行数为0，在中间
返回的行数为0，在后面
返回的行数全部为0

这几种情况都可以通过精心设计测试数据，融入到 1～9 测试用例中，比如返回的行数全部为0就是XXXX，行数相同的一种特例。中间/前面返回的行数为0可以体现在xxx,行数完全不同或xxx,行数部分不同中。

我觉得可以的。这边我比较建议你把和排序本身相关的挪过去 heap 那边。然后这边集中测试不同行数，以及交叉情况的用例

longyue0521 · 2023-03-11T11:03:30Z

internal/merger/sortmerger/merger_test.go

+			},
+		},
+		{
+			name: "排序列id ASC,name ASC_每个sqlRows行数不同_全部交叉读取各个sqlRows",


每个slqRows返回行数都不同

longyue0521 · 2023-03-11T11:03:59Z

internal/merger/sortmerger/merger_test.go

+			},
+		},
+		{
+			name: "排序列id ASC,name DESC_每个sqlRows行数相同_全部交叉读取各个sqlRows",


每个slqRows返回行数都相同

longyue0521 · 2023-03-11T11:10:46Z

internal/merger/sortmerger/merger_test.go

+			},
+		},
+		{
+			name: "排序列name ASC,id DESC(排序列出现的顺序和sqlRows的顺序不同)_返回的行数全部相同_全部交叉读取各个sqlRows",


你把各个用例的name都抽取出来，放在一起比较一下，name的命名规则/模式要保持一致，请仔细校验后续全部name，将其改为统一命名形式，保证测试的可读性。

通过仔细阅读这些name看看你还缺哪些用例，比如：id DESC, name DESC

名字可以取得比较简洁，然后如果觉得名字不够细节，那么在前面加上一个注释也可以。

longyue0521 · 2023-03-11T11:12:45Z

internal/merger/sortmerger/merger_test.go

+			},
+		},
+		{
+			name: "排序列id ASC,name ASC_其中有一个sqlRows的数据为空的在中间_部分sqlRows返回行数相同_全部交叉读取各个sqlRows",


其中有一个sqlRows的数据为空的在中间 去掉

longyue0521 · 2023-03-12T03:26:36Z

internal/merger/sortmerger/merger_test.go

+			},
+		},
+		{
+			name: "部分sqlRows返回的行数为0，在前面",


按照我上一个留言，将”返回行为0“出现的位置，融入到之前的测试用例中。再次检查name的命名，使其保持一致、可读性。

longyue0521 · 2023-03-12T03:29:42Z

internal/merger/sortmerger/merger_test.go

+	assert.Equal(ms.T(), cols, columns)
+}
+
+func (ms *MergerSuite) TestRows_Close() {


设计测试用例，覆盖Close返回mulitierror的情况。

longyue0521 · 2023-03-12T03:54:30Z

@juniaoshaonian @flycash

当前实现缺少的测试：

缺少Err方法的测试
缺少Err方法与Next/Scan/Close方法各种交叉调用的测试
缺少Close返回multierror的场景测试
并发测试——merger.Rows各个方法在并发场景下的测试
缺少边界情况的测试，即确认不同的方法调用顺序导致merger.Rows的状态迁移是否明确，例如：拿到merger.Rows
- 直接Scan会发生什么？
- 假设sqlRows返回行为都为0，调用Next后，Err()返回什么？
- Close后再次调用Scan会发生什么？
- 等等。

@juniaoshaonian 读源码或者实际测试一下sql.Rows在上面各种情况的表现，尽可能做到与sql.Rows语义一致。

flycash · 2023-03-12T05:37:21Z

这边测试的话，拆成不同的方法，比如说 Err 一个方法， Err 和 Scan 交叉的再拆出来一个方法。这样我们的 test case 的名字就不至于非常长。这种组织方式，也更加容易理解。

另外有一个测试，就是 Err 不会 nil 之后再次尝试 Scan 会发生什么？或者说在 Scan 中间发现了 Err，继续 Scan 会怎么办。

juniaoshaonian · 2023-03-13T14:51:22Z

关于并发测试应该怎么测，我的理解我们保证的是单个Next或者Scan，他们两和起来不是并发安全的，sql.rows里面的Next，Scan方法都加了读写锁。但是这个咋测单个Next和Scan是并发安全的，大佬们有没有想法聊聊呀。

flycash · 2023-03-13T14:53:41Z

并发测试这边不需要花太大精力，靠 review 来。其实大部分情况下 review 能够保证基本正确，尤其是在这里加读写锁，并发逻辑并不复杂。而奇诡的并发，我只能说你测也测不出来，看也看不出来。

并发这种就有一点，尽人事看天命的感觉

flycash · 2023-03-13T14:56:15Z

合并一下最新代码

longyue0521 requested changes Mar 4, 2023

View reviewed changes

internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved

internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved

internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved

flycash requested changes Mar 6, 2023

View reviewed changes

longyue0521 requested changes Mar 7, 2023

View reviewed changes

longyue0521 requested changes Mar 8, 2023

View reviewed changes

flycash requested changes Mar 8, 2023

View reviewed changes

flycash reviewed Mar 8, 2023

View reviewed changes

internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved

internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved

internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved

longyue0521 requested changes Mar 9, 2023

View reviewed changes

flycash approved these changes Mar 9, 2023

View reviewed changes

longyue0521 requested changes Mar 9, 2023

View reviewed changes

juniaoshaonian requested a review from longyue0521 March 9, 2023 10:44

longyue0521 requested changes Mar 9, 2023

View reviewed changes

juniaoshaonian requested a review from longyue0521 March 9, 2023 13:39

longyue0521 requested changes Mar 9, 2023

View reviewed changes

juniaoshaonian requested a review from longyue0521 March 10, 2023 02:04

longyue0521 requested changes Mar 10, 2023

View reviewed changes

flycash linked an issue Mar 10, 2023 that may be closed by this pull request

分库分表：merger 排序实现 #141

Closed

juniaoshaonian requested a review from longyue0521 March 11, 2023 04:58

longyue0521 requested changes Mar 11, 2023

View reviewed changes

longyue0521 requested changes Mar 12, 2023

View reviewed changes

juniaoshaonian closed this Mar 13, 2023

longyue0521 mentioned this pull request Mar 14, 2023

分库分表： merger排序实现 #169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

分库分表：merger 排序实现 #166

分库分表：merger 排序实现 #166

juniaoshaonian commented Mar 4, 2023

codecov-commenter commented Mar 4, 2023 •

edited by codecov bot

flycash commented Mar 4, 2023

flycash commented Mar 4, 2023

flycash commented Mar 4, 2023

flycash Mar 6, 2023

juniaoshaonian commented Mar 7, 2023

flycash left a comment

longyue0521 Mar 10, 2023

juniaoshaonian Mar 10, 2023 •

edited

longyue0521 Mar 10, 2023

juniaoshaonian Mar 10, 2023

juniaoshaonian Mar 10, 2023

longyue0521 Mar 10, 2023

juniaoshaonian Mar 10, 2023

longyue0521 Mar 11, 2023

flycash Mar 11, 2023

juniaoshaonian Mar 12, 2023 •

edited

juniaoshaonian Mar 12, 2023

longyue0521 Mar 12, 2023

flycash Mar 13, 2023

longyue0521 Mar 11, 2023

longyue0521 Mar 11, 2023

longyue0521 Mar 11, 2023

flycash Mar 11, 2023

longyue0521 Mar 11, 2023

longyue0521 Mar 12, 2023

longyue0521 Mar 12, 2023

longyue0521 commented Mar 12, 2023

flycash commented Mar 12, 2023

juniaoshaonian commented Mar 13, 2023

flycash commented Mar 13, 2023

flycash commented Mar 13, 2023

分库分表：merger 排序实现 #166

分库分表：merger 排序实现 #166

Conversation

juniaoshaonian commented Mar 4, 2023

codecov-commenter commented Mar 4, 2023 • edited by codecov bot

Codecov Report

flycash commented Mar 4, 2023

flycash commented Mar 4, 2023

flycash commented Mar 4, 2023

Choose a reason for hiding this comment

juniaoshaonian commented Mar 7, 2023

flycash left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juniaoshaonian Mar 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juniaoshaonian Mar 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

longyue0521 commented Mar 12, 2023

flycash commented Mar 12, 2023

juniaoshaonian commented Mar 13, 2023

flycash commented Mar 13, 2023

flycash commented Mar 13, 2023

codecov-commenter commented Mar 4, 2023 •

edited by codecov bot

juniaoshaonian Mar 10, 2023 •

edited

juniaoshaonian Mar 12, 2023 •

edited