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

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

Merged
merged 8 commits into from
Mar 16, 2023
Merged

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

merged 8 commits into from
Mar 16, 2023

Conversation

juniaoshaonian
Copy link
Collaborator

合并了一下代码

@codecov
Copy link

codecov bot commented Mar 14, 2023

Codecov Report

Merging #169 (f24bb28) into dev (29ec110) will increase coverage by 1.12%.
The diff coverage is 91.35%.

@@            Coverage Diff             @@
##              dev     #169      +/-   ##
==========================================
+ Coverage   79.51%   80.64%   +1.12%     
==========================================
  Files          30       32       +2     
  Lines        2309     2552     +243     
==========================================
+ Hits         1836     2058     +222     
- Misses        397      412      +15     
- Partials       76       82       +6     
Impacted Files Coverage Δ
sharding_db.go 82.75% <ø> (ø)
internal/merger/batchmerger/merger.go 70.96% <33.33%> (-1.92%) ⬇️
internal/merger/sortmerger/merger.go 91.30% <91.30%> (ø)
internal/merger/sortmerger/heap.go 96.96% <96.96%> (ø)

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

internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger.go Show resolved Hide resolved
internal/merger/sortmerger/merger.go Show resolved Hide resolved
internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved
sharding_db.go Show resolved Hide resolved
@longyue0521
Copy link
Collaborator

@juniaoshaonian 学习一下git rebase 这样你应该就不用关闭之前的PR #166

@Stone-afk
Copy link
Collaborator

mark

internal/merger/internal/errs/error.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/heap.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/heap_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
internal/merger/sortmerger/merger.go Show resolved Hide resolved
internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
@juniaoshaonian
Copy link
Collaborator Author

juniaoshaonian commented Mar 15, 2023

大佬们关于Next遍历完自动调用close方法的问题,你们觉得现在要怎么处理

func(rs *Rows) nextLocked() (doClose, ok bool) {
	if rs.closed {
		return false, false
	}
	rs.dc.Lock()
	defer rs.dc.Unlock()
	if rs.lastcols == nil {
		rs.lastcols = make([]driver.Value, len(rs.rowsi.Columns()))
	}
	//当遍历到最后一行时会返回EOF这个错误
	rs.lasterr = rs.rowsi.Next(rs.lastcols)
	if rs.lasterr != nil {
        
		if rs.lasterr != io.EOF {
			return true, false
		}
		nextResultSet, ok := rs.rowsi.(driver.RowsNextResultSet)
		if !ok {
			return true, false
		}
        // 如果是EOF错误并且没有下一个结果集就会把他关闭 
		if !nextResultSet.HasNextResultSet() {
			doClose = true
		}
		return doClose, false
	}
	return false, true
}
// dbClose为true就会调用close方法 

@juniaoshaonian
Copy link
Collaborator Author

还有Columns方法,一个sql.Rows遍历完如果再调用Columns方法会直接报连接已关闭的错误,我的想法是如果遇到连接已关闭的错误就继续下一个sql.Rows。但是如果是其他错误应该怎么办,直接返回还是说继续遍历下一个。还是说遇到错误直接不管一直遍历下去直到最后一个,返回最后一个的错误?

@juniaoshaonian
Copy link
Collaborator Author

@longyue0521 @flycash

@juniaoshaonian juniaoshaonian requested review from flycash and longyue0521 and removed request for flycash March 15, 2023 09:50
@longyue0521
Copy link
Collaborator

大佬们关于Next遍历完自动调用close方法的问题,你们觉得现在要怎么处理

func(rs *Rows) nextLocked() (doClose, ok bool) {
	if rs.closed {
		return false, false
	}
	rs.dc.Lock()
	defer rs.dc.Unlock()
	if rs.lastcols == nil {
		rs.lastcols = make([]driver.Value, len(rs.rowsi.Columns()))
	}
	//当遍历到最后一行时会返回EOF这个错误
	rs.lasterr = rs.rowsi.Next(rs.lastcols)
	if rs.lasterr != nil {
        
		if rs.lasterr != io.EOF {
			return true, false
		}
		nextResultSet, ok := rs.rowsi.(driver.RowsNextResultSet)
		if !ok {
			return true, false
		}
        // 如果是EOF错误并且没有下一个结果集就会把他关闭 
		if !nextResultSet.HasNextResultSet() {
			doClose = true
		}
		return doClose, false
	}
	return false, true
}
// dbClose为true就会调用close方法 

按之前聊过的,尽可能与sql.Rows的各个方法语义保持一致.

在单结果集下,sortmerger.Rows.Next在非异常情况下返回false表明内部heap中没有预Scan的行了,而在预Scan之前都会先调用一下sql.Rows.Next如果其返回false也就是会自动关闭,所以现在来看sortmerger.Rows.Next可以说是做到了下面这个部分:

  nextResultSet, ok := rs.rowsi.(driver.RowsNextResultSet)
  if !ok {
  	return true, false
  }

sortmerger.Rows.Next在异常情况下返回false即内部在迭代sql.Rows时或者预Scan时报错的情况下,也设置了r.lasterr并执行了r.Close可以说是做到了下面这部分:

if rs.lasterr != io.EOF {

  	return true, false
  }

在多结果集的情况,sortmerger.Rows暂时不支持所以没有处理多结果集的情况.多结果集处理起来也有坑:假设从三个sqlRows取数据,每个sqlRows有2个结果集.sqlRows1的第一个结果集最先被取完,这时是直接取其下一个结果集的数据还是等待sqlRows2和3的第一个结果集都取完,然后调用三者的NextResultSet再一起从各自第二个结果集里取数据? 简单描述就是多个sqlRows的多个结果集之间数据能否交叉取?

@longyue0521
Copy link
Collaborator

还有Columns方法,一个sql.Rows遍历完如果再调用Columns方法会直接报连接已关闭的错误,我的想法是如果遇到连接已关闭的错误就继续下一个sql.Rows。但是如果是其他错误应该怎么办,直接返回还是说继续遍历下一个。还是说遇到错误直接不管一直遍历下去直到最后一个,返回最后一个的错误?

这么做确实可以,但也依赖于一个假设——所有sql.Rows的列都是相同的,之所以称之为假设,因为在NewSortMerger的时候并没有检查各个sqlRows的列是否相同,可以考虑把这个假设变为约束同时把你上面提到的如果是其他错误应该怎么办前置到NewSortMerger阶段,在检查各个sqlRows的列是否相同的同时在Rows内部缓存一份列集及其类型,这样就与底层sqlRows解耦了,可以根据sortmerger.Rows内部的状态来定义返回而不再依赖于底层sqlRows以及处理其error

internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger.go Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
@longyue0521
Copy link
Collaborator

@juniaoshaonian 请保持PR的单一性,在这个PR中只修改sortmerger相关代码,把你bachmerger相关代码的修改revert回去,另外开PR处理.不要混在一起改.

@juniaoshaonian
Copy link
Collaborator Author

@juniaoshaonian 请保持PR的单一性,在这个PR中只修改sortmerger相关代码,把你bachmerger相关代码的修改revert回去,另外开PR处理.不要混在一起改.

我改了一下errror的名字,和加了err()方法,让他们不报错

@longyue0521
Copy link
Collaborator

@flycash
Copy link
Contributor

flycash commented Mar 16, 2023

  • 多结果集的问题:首先第一个是 longyue 提到的,我们难以确定是不是大家的第一个结果集都处理完了,再同时开启第二个结果集;第二点是我认为用户不应该使用多结果集,因为多结果集还涉及到两次查询列不一样,比如说 select a from; select b from; 甚至于 select 和 update 混用;第三则是,按照道理来说,sql.Rows 内部已经尝试了 next result set,我们自己就不处理了。可以说,多结果集情况下,我们依赖于 sql.Rows 本身的语义。那么也就是如果全部结果集都相等,那么排序就是全部结果集一起参与排序;
  • 所有结果的列都是一样的:这个是非常强的假设,或者说是我们这个 merger 运作的基石。虽然理论上来说大家列不一样,但是排序列都一样的话我们也能处理,但是这个场景不对。在我们预期中,这个东西就应该是给相似的查询用的,也就是返回的列必须都一样。甚至于,我都认为这是一个连检测都不需要检测的假设,因为用户但凡用这个merger,他就应该知道如果列不一样或者排序列不一样会有什么奇怪的事情发生。毕竟这不是一个直面普通开发的东西,是一个给高级开发人员用的;

#169 (comment) 这个地方是要加并发测试吗?@longyue0521?

@longyue0521
Copy link
Collaborator

第三则是,按照道理来说,sql.Rows 内部已经尝试了 next result set,我们自己就不处理了。可以说,多结果集情况下,我们依赖于 sql.Rows 本身的语义。那么也就是如果全部结果集都相等,那么排序就是全部结果集一起参与排序;

sql.Rows只是调用HasNextResultSet检测一下是否有下一个结果集并不会自动跳到下一结果集,需要用户显示调用sql.Rows.NextResultSet方法跳到下一个结果集。

所有结果的列都是一样的:这个是非常强的假设,或者说是我们这个 merger 运作的基石。虽然理论上来说大家列不一样,但>是排序列都一样的话我们也能处理,但是这个场景不对。在我们预期中,这个东西就应该是给相似的查询用的,也就是返回的>列必须都一样。甚至于,我都认为这是一个连检测都不需要检测的假设,因为用户但凡用这个merger,他就应该知道如果列不>一样或者排序列不一样会有什么奇怪的事情发生。毕竟这不是一个直面普通开发的东西,是一个给高级开发人员用的;

好的,在顶层抽象Merger接口加个注释怎么样,一方面介绍Merger的作用,另一方面提示一下:[]sql.Rows中每个sql.Rows仅支持单个结果集且每个sql.Rows中列集必须完全相同

#169 (comment) 这个地方是要加并发测试吗?@longyue0521?

不是,是为了回答这个问题 #169 (comment)

我认为现在sortmerger.Rows.Next的语义符合sql.Rows.Next的语义

@flycash
Copy link
Contributor

flycash commented Mar 16, 2023

好的,在顶层抽象Merger接口加个注释怎么样,一方面介绍Merger的作用,另一方面提示一下:[]sql.Rows中每个sql.Rows仅支持单个结果集且每个sql.Rows中列集必须完全相同。

OKKK

internal/merger/sortmerger/merger.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
internal/merger/sortmerger/merger_test.go Outdated Show resolved Hide resolved
@longyue0521
Copy link
Collaborator

好的,在顶层抽象Merger接口加个注释怎么样,一方面介绍Merger的作用,另一方面提示一下:[]sql.Rows中每个sql.Rows仅支持单个结果集且每个sql.Rows中列集必须完全相同。

OKKK

@juniaoshaonian 按上面的描述添加一下注释,再我把上面提到注释改了,这个PR就OK了

@longyue0521
Copy link
Collaborator

@juniaoshaonian 在最顶层Rows接口上也加一个注释:各方法用法及语义尽可能与sql.Rows

@flycash flycash merged commit 9796f8e into ecodeclub:dev Mar 16, 2023
@flycash flycash linked an issue Mar 16, 2023 that may be closed by this pull request
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.

分库分表:merger 排序实现
4 participants