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

Copr: implement sub_datetime_and_duration and sub_datetime_and_string #4269

Merged
merged 10 commits into from Mar 7, 2019

Conversation

Projects
None yet
5 participants
@koushiro
Copy link
Contributor

koushiro commented Feb 24, 2019

What have you changed? (mandatory)

Implement:

  • sub_datetime_and duration
  • sub_datetime_and_string
  • sub_time_datetime_null

Improve:

  • add_datetime_and_string

What are the type of the changes? (mandatory)

  • Improvement

How has this PR been tested? (mandatory)

unit test.

Does this PR affect documentation (docs) update? (mandatory)

no

Does this PR affect tidb-ansible update? (mandatory)

no

Refer to a related PR or issue link (optional)

#3275

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

koushiro added some commits Feb 24, 2019

Implement sub_datetime_xxx
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
Merge remote-tracking branch 'upstream/master' into sub_datetime
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Feb 24, 2019

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

@AndreMouche
Copy link
Member

AndreMouche left a comment

Rest LGTM

let s = ::std::str::from_utf8(&arg1)?;
let arg1 = MyDuration::parse(&arg1, Time::parse_fsp(s))?;
let overflow = Error::overflow("TIME", &format!("({} - {})", &arg0, &arg1));
let res = match arg0.into_owned().checked_sub(&arg1) {

This comment has been minimized.

@AndreMouche

AndreMouche Feb 25, 2019

Member

Should we consider to convert it to type Datetime if it is not? @zz-jason

This comment has been minimized.

@XuHuaiyu

This comment has been minimized.

@koushiro

koushiro Feb 27, 2019

Author Contributor

Like that?

let mut res = ...;
res.set_time_type(TimeType::DateTime)?;
ctx: &mut EvalContext,
_row: &[Datum],
) -> Result<Option<Cow<'a, Time>>> {
Ok(Some(Cow::Owned(mysql::time::zero_datetime(ctx.cfg.tz))))

This comment has been minimized.

@AndreMouche

AndreMouche Feb 25, 2019

Member

I think we should return None here @zz-jason

This comment has been minimized.

@XuHuaiyu
Fix
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@AndreMouche

This comment has been minimized.

Copy link
Member

AndreMouche commented Feb 27, 2019

Any update?

Fix time type; Format code
Signed-off-by: koushiro <koushiro.cqx@gmail.com>
@koushiro

This comment has been minimized.

Copy link
Contributor Author

koushiro commented Feb 27, 2019

@AndreMouche PTAL again.

@AndreMouche
Copy link
Member

AndreMouche left a comment

LGTM

@AndreMouche

This comment has been minimized.

Copy link
Member

AndreMouche commented Feb 28, 2019

@rleungx

rleungx approved these changes Mar 4, 2019

@rleungx rleungx added S: LGT2 and removed S: Waiting labels Mar 4, 2019

rleungx and others added some commits Mar 6, 2019

@AndreMouche

This comment has been minimized.

Copy link
Member

AndreMouche commented Mar 7, 2019

/test

@AndreMouche AndreMouche merged commit 074f2d2 into tikv:master Mar 7, 2019

2 checks passed

DCO All commits are signed off!
Details
idc-jenkins-ci/test Jenkins job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.