分库分表：merger 排序实现#166
Conversation
Codecov Report
@@ 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
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
这个思路是没问题，不过有几个点：
数据类型的转换，在 sql 包里面是 convertAssignRows 这个东西。所以基本上就是抄一遍这个方法。实在无力吐槽这个关键的方法它居然是私有的。 这里面是有隐患的，就是你在 Next 的时候就需要取出来数据，然后临时存着。理论上来说用 []byte 不会有什么问题，但是我觉得这里面可能有不少的坑。 |
|
你考虑这样一个场景，就是分页的场景，比如说排序之后取（LIMIT 100,10），那么你完全不需要全部排序一遍，而是就只排序一部分。 |
|
那么 Scan 里面，就调用你抄过来的那个 ConvertAndAssign 就可以。 |
internal/merger/sortmerger/merger.go
Outdated
| DESC Order = false | ||
| ) | ||
|
|
||
| type Merger[T any] struct { |
There was a problem hiding this comment.
这个 Merger 肯定不可能是泛型的。注意，在 merger 这个包，它完全没有任何将数据转化为结构体的概念。
|
迭代过程中的错误处理我是这样搞的，无论是排序过程中遇到的，还是sql.rows的Next方法遇到的错误。都会结束迭代，调用close方法然后返回false。 |
flycash
left a comment
flycash
left a comment
There was a problem hiding this comment.
@longyue0521 @Stone-afk 麻烦看看。@Stone-afk 你会是主要用户，所以你看看是否符合你的预期
| }{ | ||
|
|
||
| { | ||
| name: "排序列id ASC_多个sqlRows_每个sqlRows返回行数相同_交叉读取各个sqlRows", |
There was a problem hiding this comment.
你对交叉读的定义是什么? 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 叫什么?
明确定义后,自行校对一下测试用例是否符合你的定义.
There was a problem hiding this comment.
就两种，顺序一个sql.rows读完再读另外一个直至全部读完，交叉就是一个读了部分再读其他
There was a problem hiding this comment.
可以，顺序读定义明确；交叉要不要分为部分交叉和全交叉？为什么不要？
There was a problem hiding this comment.
那我再加一个部分交叉的例子：有一行一次性全部读完。两行的交叉着读
| }, | ||
| }, | ||
| { | ||
| name: "排序列id ASC_多个sqlRows_部分sqlRows返回行数相同_顺序读取各个sqlRows", |
There was a problem hiding this comment.
既然你不想重新设计这个测试用例(#166 (comment)) ,那就补充缺少的测试用例以覆盖 —— 各个sqlRows返回行数都不同的场景
There was a problem hiding this comment.
我下面我记得写了一个呀
| }, | ||
| }, | ||
| { | ||
| name: "排序列id ASC_部分sqlRows返回行数相同_顺序读取各个sqlRows", |
There was a problem hiding this comment.
我觉得 longyue 的想法是说，在这个测试用例设计上，有几个因素会影响 Next 在不同的 sql.Rows 里面切换：
- 完全交叉、部分交叉和完全不交叉
- 行数，部分数量比较少的行会提前结束，那么其它 Rows 依旧能够正确被读取完毕。那么极端的情况就是大家行数都不同，行数都相同；再结合行数可以为0，那么可以是头几个为行数为0，后几个行数为0，中间几个行数为0
有一些你在这个测试用例里面可以不体现的，就是排序列是正序还是倒序，以及多少个排序列。这部分是你的 heap 实现来保证的，这边有一个简单的例子就差不多——主要是确保 Heap 和 Next 运作正常。
There was a problem hiding this comment.
- 完全交叉，行数相同
- 完全交叉，行数部分不同
- 完全交叉，行数完全不同
- 部分交叉，行数相同
- 部分交叉, 行数部分不同
- 部分交叉，行数完全不同
- 顺序读，行数相同
- 顺序读，行数部分不同
- 顺序读，行数完全不同
// 这边就不分交叉读什么的了，上面已经讨论过了，以完全交叉读为例 - 返回的行数为0，在前面
- 返回的行数为0，在中间
- 返回的行数为0，在后面
- 返回的行数全部为0
- 排序列返回的顺序和数据库里的字段顺序不一致
There was a problem hiding this comment.
有一些你在这个测试用例里面可以不体现的，就是排序列是正序还是倒序，以及多少个排序列。这部分是你的 heap 实现来保证的，这边有一个简单的例子就差不多——主要是确保 Heap 和 Next 运作正常。
我赞同 @flycash的建议，但这是有前提的——你抽取出去的heap有对应的测试覆盖来保证。现在的情况是抽取出的heap没有对应用的测试测试，所以这里只能用类似于”集成测试“或粒度更大的单元测试的概念来保证正确性。
返回的行数为0，在前面
返回的行数为0，在中间
返回的行数为0，在后面
返回的行数全部为0
这几种情况都可以通过精心设计测试数据，融入到 1～9 测试用例中，比如返回的行数全部为0就是XXXX，行数相同的一种特例。中间/前面返回的行数为0可以体现在xxx,行数完全不同或xxx,行数部分不同中。
There was a problem hiding this comment.
我觉得可以的。这边我比较建议你把和排序本身相关的挪过去 heap 那边。然后这边集中测试不同行数，以及交叉情况的用例
| }, | ||
| }, | ||
| { | ||
| name: "排序列id ASC,name ASC_每个sqlRows行数不同_全部交叉读取各个sqlRows", |
| }, | ||
| }, | ||
| { | ||
| name: "排序列id ASC,name DESC_每个sqlRows行数相同_全部交叉读取各个sqlRows", |
| }, | ||
| }, | ||
| { | ||
| name: "排序列name ASC,id DESC(排序列出现的顺序和sqlRows的顺序不同)_返回的行数全部相同_全部交叉读取各个sqlRows", |
There was a problem hiding this comment.
你把各个用例的name都抽取出来，放在一起比较一下，name的命名规则/模式要保持一致，请仔细校验后续全部name，将其改为统一命名形式，保证测试的可读性。
通过仔细阅读这些name看看你还缺哪些用例，比如：id DESC, name DESC
There was a problem hiding this comment.
名字可以取得比较简洁，然后如果觉得名字不够细节，那么在前面加上一个注释也可以。
| }, | ||
| }, | ||
| { | ||
| name: "排序列id ASC,name ASC_其中有一个sqlRows的数据为空的在中间_部分sqlRows返回行数相同_全部交叉读取各个sqlRows", |
There was a problem hiding this comment.
其中有一个sqlRows的数据为空的在中间 去掉
| }, | ||
| }, | ||
| { | ||
| name: "部分sqlRows返回的行数为0，在前面", |
There was a problem hiding this comment.
按照我上一个留言，将”返回行为0“出现的位置，融入到之前的测试用例中。再次检查name的命名，使其保持一致、可读性。
| assert.Equal(ms.T(), cols, columns) | ||
| } | ||
|
|
||
| func (ms *MergerSuite) TestRows_Close() { |
There was a problem hiding this comment.
设计测试用例，覆盖Close返回mulitierror的情况。
|
当前实现缺少的测试：
@juniaoshaonian 读源码或者实际测试一下sql.Rows在上面各种情况的表现，尽可能做到与sql.Rows语义一致。 |
|
这边测试的话，拆成不同的方法，比如说 Err 一个方法， Err 和 Scan 交叉的再拆出来一个方法。这样我们的 test case 的名字就不至于非常长。这种组织方式，也更加容易理解。 另外有一个测试，就是 Err 不会 nil 之后再次尝试 Scan 会发生什么？或者说在 Scan 中间发现了 Err，继续 Scan 会怎么办。 |
|
关于并发测试应该怎么测，我的理解我们保证的是单个Next或者Scan，他们两和起来不是并发安全的，sql.rows里面的Next，Scan方法都加了读写锁。但是这个咋测单个Next和Scan是并发安全的，大佬们有没有想法聊聊呀。 |
|
并发测试这边不需要花太大精力，靠 review 来。其实大部分情况下 review 能够保证基本正确，尤其是在这里加读写锁，并发逻辑并不复杂。而奇诡的并发，我只能说你测也测不出来，看也看不出来。 并发这种就有一点，尽人事看天命的感觉 |
|
合并一下最新代码 |
4 participants
scan方法我没有实现