Skip to content

introduce a new function signature ScalarFuncSig_TimeIsNullOnNotNullCol#121

Closed
qw4990 wants to merge 3 commits intopingcap:masterfrom
qw4990:fix-9763
Closed

introduce a new function signature ScalarFuncSig_TimeIsNullOnNotNullCol#121
qw4990 wants to merge 3 commits intopingcap:masterfrom
qw4990:fix-9763

Conversation

@qw4990
Copy link
Copy Markdown

@qw4990 qw4990 commented Jun 10, 2019

Introduce a new function signature ScalarFuncSig_TimeIsNullOnNotNullCol to solve this MySQL-compatibility problem(pingcap/tidb#9763);

Comment thread src/protobuf/select.rs Outdated
@@ -1,4 +1,4 @@
// This file is generated by rust-protobuf 2.5.0. Do not edit
// This file is generated by rust-protobuf 2.6.1. Do not edit
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@breeswish Can TiKV use the rust code generated by rust-protobuf 2.6.1?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It seems that cargo upgrades the rust-protobuf dependency automatically.
It should be OK and here is another example 535e1ab#diff-9ab65b665f1b3a0f209eddc266d5a70eL1 .
@breeswish Please help to confirm it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is there 5000+ lines of difference 🤔

Copy link
Copy Markdown
Author

@qw4990 qw4990 Jun 11, 2019

Choose a reason for hiding this comment

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

It seems that the rust code format changed...
The Format of this PR is different from the previous PR.
The previous PR changed the format with 8000+ lines of difference, and this PR changes the format back, so there are so many lines of difference.
So which one is better... @breeswish

Copy link
Copy Markdown
Member

@breezewish breezewish Jun 11, 2019

Choose a reason for hiding this comment

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

@BusyJay PTAL, it is ok with this change? ( I mean 2.5.0 and 2.6.1 )

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@BusyJay Should we commit locks?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I generated rust code again with the previous PR's protobuf version(2.5.0).
PTAL @BusyJay

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@breeswish No, we don't. Better lock it inside Cargo.toml.
@qw4990 Have you tested it with TiKV? I'm not sure whether it breaks anything if the version doesn't match.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I tried to do some tests, but there are some things (about protoc version and prost) wrong in TiKV to let it can't upgrade tipb(tikv/tikv#4864), and TiKV has hard-written a specific version of tipb now... @BusyJay

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Since TiKV locks tipb to a specific version, I prefer to merge this PR first to let TiDB(pingcap/tidb#10753) can use this new function and then solve problems in TiKV later.

@qw4990
Copy link
Copy Markdown
Author

qw4990 commented Jun 20, 2019

Wait for pingcap/tidb#10790 and use it to solve this problem instead of introducing a new function signature.

@qw4990 qw4990 closed this Jun 20, 2019
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.

4 participants