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

database/gdb: enhance the underlying data transformation when querying data #3557

Closed
wants to merge 44 commits into from

Conversation

wln32
Copy link
Member

@wln32 wln32 commented May 1, 2024

本次改动只修改了Scan方法的底层的类型转换实现,会走新的逻辑,依旧是先把数据转换到map,然后在转到结构体上,

和之前的行为保持一致,不会导致不兼容。
从map到结构体的转换和之前不同的是,由于在底层转换时已经转成了具体类型,不需要调用gconv来做转换,可以直接使用循环赋值

====更新7.5
对于本次更新增加一个开关,打开时走新的逻辑,关闭时还是走旧逻辑
新增加自定义字段转换的特性,方便用户平稳过渡
1.RegisterDatabaseConvertFunc 为某个驱动(比如mysql)的某个类型(比如decimal)注册自定义转换函数
2.RegisterGoTypeConvertFunc为go语言的类型添加自定义转换函数,且优先级比1高


未修改之前的

1

修改后的

3333


性能方面大幅提升,如果是直接一步赋值到结构体上,只需要600ms,但这样会导致有些api不兼容

一些建议

  1. 不建议对一整个结构体实现UnmarshalValue接口来做赋值,例如

3

  1. 和orm的转换有关的接口,应当实现Scan和Value这两个接口来做,和标准库保持一致

  2. 建议废弃掉All,One之类的方法,一点都不好用,如果查询的是单个字段,或者是一个[]int,还需要再次转换才能使用,不如直接把Scan方法加强,支持 int , []int 这种类型的参数,一步赋值到目标字段,而不是走中间变量来回重复的转换

  3. 关于HookFuncSelect 的Next方法的返回值,不应该把原始的数据结构暴漏出去让用户操作,应当封装到结构体内部,通过方法去操作


@wln32 wln32 linked an issue May 1, 2024 that may be closed by this pull request
@wln32 wln32 requested review from gqcn and hailaz and removed request for gqcn May 1, 2024 08:06
@wln32 wln32 changed the title database/gdb: Improve/gdb scan typeconvert database/gdb: enhance the underlying data transformation when querying data May 1, 2024
Copy link
Member

@houseme houseme left a comment

Choose a reason for hiding this comment

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

补充一下公共方法的注释

@@ -1621,6 +1622,13 @@ func Test_Types(t *testing.T) {
Bit int8
TinyInt bool
}
now, err := db.Query(ctx, "select now();")
t.Log("mysql now=", now, err)
Copy link
Member

Choose a reason for hiding this comment

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

为啥要在单测里打日志,下面也一样

Copy link
Member Author

Choose a reason for hiding this comment

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

为啥要在单测里打日志,下面也一样

这个当然是有原因的,可以看issue #3558 关于gtime的bug,等下就把他删掉

}

func Test_Core_Convert_Custom(t *testing.T) {

Copy link
Member

Choose a reason for hiding this comment

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

函数开头结尾为啥要带空行,有哪个标准是这样做的吗

Copy link
Member

Choose a reason for hiding this comment

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

函数开头结尾为啥要带空行,有哪个标准是这样做的吗

可以有,也可以开启golangci-lint 检测拦截这种处理

Copy link
Member

Choose a reason for hiding this comment

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

可以有也行,保持一致就行

Copy link
Member

Choose a reason for hiding this comment

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

可以有也行,保持一致就行
原来是不留空行的,建议删除了

Comment on lines 35 to 43
if i > 0 {
if v.Kind() == reflect.Pointer {
if v.IsNil() {
v.Set(reflect.New(v.Type().Elem()))
}
v = v.Elem()
}
}
v = v.Field(x)
Copy link
Member

Choose a reason for hiding this comment

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

控制反转一下这边的代码,层级太深了

Copy link
Member Author

Choose a reason for hiding this comment

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

控制反转一下这边的代码,层级太深了

ok

if len(c.StructFieldIndex) == 1 {
return structValue.Field(c.StructFieldIndex[0])
}
v := structValue
Copy link
Member

Choose a reason for hiding this comment

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

为啥用变量再赋值一次

Copy link
Member Author

Choose a reason for hiding this comment

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

为啥用变量再赋值一次

这里当时偷懒了,因为这个方法的逻辑是从标准库cv过来的

Comment on lines 247 to 282
if ok {
// It needs to be deleted, otherwise it will cause
// conflicts as long as the struct name is the same within different functions during testing
defer tablesMap.Delete(tableName)

table = tableValue.(*Table)
structPointerValue := reflect.ValueOf(pointer).Elem()

var structValue = reflect.New(elemType)
// UnmarshalValue
fn, ok := structValue.Interface().(iUnmarshalValue)
if ok {
err = fn.UnmarshalValue(one)
if err != nil {
return err
}
structValue = structValue.Elem()
} else {
structValue = structValue.Elem()
for _, field := range table.fields {
fieldValue := field.GetReflectValue(structValue)
value := one[field.ColumnFieldName]
if value == nil {
continue
}
fieldValue.Set(reflect.ValueOf(value.Val()))
}
}
if elemIsPtr {
structValue = structValue.Addr()
}
structPointerValue.Set(structValue)
} else {
if err = one.Struct(pointer); err != nil {
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

if !ok {return}
....

Copy link
Member Author

Choose a reason for hiding this comment

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

@oldme-git 这里不能通过提前判断!ok 来返回,因为不管ok为true或者是false,都需要调用model.doWithScanStruct,来判断是否需要关联查询

Copy link
Member

Choose a reason for hiding this comment

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

在 !ok 中提前返回不可以吗

Copy link
Member Author

Choose a reason for hiding this comment

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

在 !ok 中提前返回不可以吗

不行,因为最后需要检查是否需要with关联查询,不过我已经把这个逻辑单独提取出来一个方法了,这样就可以提前返回了

@oldme-git
Copy link
Member

@gqcn @wln32 需要在 gdb 里面引入复杂的转换逻辑吗?这边我不是很了解

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@gqcn @wln32 Do you need to introduce complex conversion logic into gdb? I don't know much about this

@wln32
Copy link
Member Author

wln32 commented May 22, 2024

补充一下公共方法的注释

添加注释ok

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


Add comments about public methods

Add comment ok

@wln32
Copy link
Member Author

wln32 commented May 22, 2024

@gqcn @wln32 需要在 gdb 里面引入复杂的转换逻辑吗?这边我不是很了解

@oldme-git 原来的类型转换依赖于gconv,gconv的转换过于复杂,从数据库的数据到结构体,不需要那么复杂的转换。此次pr的代码看起来可能比较多,其实就两部分,一部分是解析结构体字段,拿到字段的tag,索引,字段类型之类的,缓存起来,还有一部分就是具体的转换函数,这部分是最多的,但也是逻辑最简单的,就是挨个判断类型,然后匹配赋值即可

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@gqcn @wln32 Do you need to introduce complex conversion logic into gdb? I don't know much about this

@oldme-git The original type conversion relies on gconv. The conversion of gconv is too complicated. From database data to structure, there is no need for such complicated conversion. The PR code this time may seem like a lot, but it actually consists of two parts. One part is to parse the structure fields, get the field tags, indexes, field types, etc., and cache them, and the other part is the specific conversion function. This It has the most parts, but it is also the simplest in logic. It is to judge the types one by one, and then match and assign values.

@gqcn
Copy link
Member

gqcn commented May 23, 2024

改动确实有点多,需要多花些时间来review,估计会挂很长一段时间。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


There are indeed a lot of changes, so it will take more time to review, and it will probably hang for a long time.

@gqcn gqcn added the slow reviewing It might be complicated or too many changes, which needs more time reviewing. label May 23, 2024
wln32 added 6 commits July 5, 2024 15:26
 - RegisterDatabaseConvertFunc 注册某个驱动(比如mysql)的数据库字段类型
 - RegisterStructFieldConvertFunc 注册某个go语言结构体的字段
2. 一些细节优化
@wln32 wln32 closed this Oct 16, 2024
@cyjaysong
Copy link
Contributor

@wln32 怎么关闭了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@wln32 Why is it closed?

@wln32
Copy link
Member Author

wln32 commented Oct 18, 2024

@wln32 怎么关闭了

这个目前看来不会被合并了,所以没什么用了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@wln32 Why is it closed?

It seems that this will not be merged at present, so it is of no use.

@cyjaysong
Copy link
Contributor

@wln32怎么关闭了

这个目前看来不会被合并了,所以没什么用了

@gqcn 老郭不是说只是审核慢么

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@wln32 why is it closed?

It seems that this will not be merged at present, so it is of no use.

@gqcn Didn’t Lao Guo say it’s just that the review is slow?

@gqcn
Copy link
Member

gqcn commented Oct 19, 2024

@wln32 @cyjaysong 只是review慢,但是代码改动优点多我每次回过头来看都很头疼。碎片时间多,每一次回头来看都是全新的开始。

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@wln32 @cyjaysong It’s just that the review is slow, but the code changes have so many advantages that I get a headache every time I look back. There is so much fragmented time, and every time you look back it is a brand new beginning.

@wln32
Copy link
Member Author

wln32 commented Oct 19, 2024

@wln32 @cyjaysong 只是review慢,但是代码改动优点多我每次回过头来看都很头疼。碎片时间多,每一次回头来看都是全新的开始。

其实有个开关可以打开或关闭新逻辑,有bug时让用户自行关闭,走旧逻辑就可以了,然后修复bug,过度几个版本就差不多了

@Issues-translate-bot
Copy link

Bot detected the issue body's language is not English, translate it automatically. 👯👭🏻🧑‍🤝‍🧑👫🧑🏿‍🤝‍🧑🏻👩🏾‍🤝‍👨🏿👬🏿


@wln32 @cyjaysong It’s just that the review is slow, but the code changes have so many advantages that I get a headache every time I look back. There is so much fragmented time, and every time you look back it is a brand new beginning.

In fact, there is a switch that can turn on or off the new logic. If there is a bug, let the user turn it off by themselves and just use the old logic. Then fix the bug and it will be almost done after a few versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement slow reviewing It might be complicated or too many changes, which needs more time reviewing.
Projects
None yet
6 participants