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

coprocessor/dag/expr: add builtin round function(RoundDec, RoundReal,… #3621

Merged
merged 3 commits into from Sep 29, 2018

Conversation

Projects
None yet
7 participants
@colinback
Copy link
Contributor

colinback commented Sep 19, 2018

… RoundWithFracxxx)

Signed-off-by: shizy shizy@cn.ibm.com

What have you changed? (mandatory)

Implement round_xxx function (round_dec, rould_real, round_with_frac_int, round_with_frac_dec, round_with_frac_real)

What are the type of the changes? (mandatory)

Improvement (non-breaking change which is an improvement to an existing feature)

How has this PR been tested? (mandatory)

Please describe the tests that you ran to verify your changes. Have you finished unit tests, integration tests, or manual tests? What additional tests would give you greater confidence in this change?

Unitest

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

NO

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

NO

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@sre-bot

This comment has been minimized.

Copy link
Collaborator

sre-bot commented Sep 19, 2018

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.

@colinback

This comment has been minimized.

Copy link
Contributor Author

colinback commented Sep 19, 2018

@solotzg @Connor1996 Based on the requirement in #3470, I implement all round_xxx func based on recent master branch. Maybe we can close #3470 and track this one.

Test result on my laptop is OK. I hope it gets same result on ci test this time.

test writebatch::bench_writebatch::bench_writebatch_4 ... ok
test writebatch::bench_writebatch::bench_writebatch_8 ... ok
test writebatch::bench_writebatch::bench_writebatch_2 ... ok
test writebatch::bench_writebatch::bench_writebatch_1 ... ok

test result: ok. 41 passed; 0 failed; 5 ignored; 0 measured; 0 filtered out

# TODO: remove above target once https://github.com/rust-lang/cargo/issues/2984 is resolved.
➜  tikv git:(Roundxxx_func)

NOTE: in the code, I still have to use name RouldReal defined in tipb. It is definitely a mistype and PR #97 to fix is already accepted. However if I update tipb version in Cargo.lock file and replace with RoundReal, I would get compile errors.

[[package]]
name = "tipb"
version = "0.0.1"
source = "git+https://github.com/pingcap/tipb.git#cacfda7d5a5e341d8cedef5fe22e11ea57b1bb96"
dependencies = [
 "protobuf 2.0.4 (registry+https://github.com/rust-lang/crates.io-index)",
]

Because it removes set_encode_type in PR #96. It seems would affect more places.

error[E0599]: no method named `set_encode_type` found for type `tipb::select::StreamResponse` in the current scope
  --> src/coprocessor/dag/dag.rs:84:16
   |
84 |         s_resp.set_encode_type(EncodeType::TypeDefault);
   |                ^^^^^^^^^^^^^^^

error: aborting due to previous error

I've no idea how to deal with this. @zz-jason any suggestion?

Sorry guys, I may not response promptly. But I will sure to check reply by the weekend.

@colinback

This comment has been minimized.

Copy link
Contributor Author

colinback commented Sep 19, 2018

Again ci test fails. Some difference between my local environment and ci test?

tikv git:(Roundxxx_func) ✗ rustc --version
rustc 1.29.0-nightly (4f3c7a472 2018-07-17)
@colinback

This comment has been minimized.

Copy link
Contributor Author

colinback commented Sep 19, 2018

thread 'raftstore_cases::test_prevote::test_prevote_reboot_minority_followers' panicked at 'called `Result::unwrap()` on an `Err` value: Grpc(BindFail("127.0.0.1", 40828))', libcore/result.rs:945:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
stack backtrace:
   0: std::sys::unix::backtrace::tracing::imp::unwind_backtrace
             at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1: std::sys_common::backtrace::print
             at libstd/sys_common/backtrace.rs:71
             at libstd/sys_common/backtrace.rs:59
   2: std::panicking::default_hook::{{closure}}
             at libstd/panicking.rs:211
   3: std::panicking::default_hook
             at libstd/panicking.rs:227
   4: std::panicking::rust_panic_with_hook
             at libstd/panicking.rs:475
   5: std::panicking::continue_panic_fmt
             at libstd/panicking.rs:390
   6: rust_begin_unwind
             at libstd/panicking.rs:325
   7: core::panicking::panic_fmt
             at libcore/panicking.rs:77
   8: core::result::unwrap_failed
             at /checkout/src/libcore/macros.rs:26
   9: <core::result::Result<T, E>>::unwrap
             at /checkout/src/libcore/result.rs:782
  10: <test_raftstore::server::ServerCluster as test_raftstore::cluster::Simulator>::run_node
             at components/test_raftstore/server.rs:191
  11: <test_raftstore::cluster::Cluster<T>>::run_node
             at ./components/test_raftstore/cluster.rs:197
  12: integrations::raftstore_cases::test_prevote::test_prevote::{{closure}}
             at tests/raftstore_cases/test_prevote.rs:104
  13: core::iter::iterator::Iterator::for_each::{{closure}}
             at /checkout/src/libcore/iter/iterator.rs:551
  14: <core::slice::Iter<'a, T> as core::iter::iterator::Iterator>::fold
             at /checkout/src/libcore/slice/mod.rs:2454
  15: core::iter::iterator::Iterator::for_each
             at /checkout/src/libcore/iter/iterator.rs:551
  16: integrations::raftstore_cases::test_prevote::test_prevote
             at tests/raftstore_cases/test_prevote.rs:104
  17: integrations::raftstore_cases::test_prevote::test_prevote_reboot_minority_followers
             at tests/raftstore_cases/test_prevote.rs:212
  18: integrations::__test::TESTS::{{closure}}
             at tests/raftstore_cases/test_prevote.rs:209
  19: core::ops::function::FnOnce::call_once
             at /checkout/src/libcore/ops/function.rs:223
  20: <F as alloc::boxed::FnBox<A>>::call_box
             at libtest/lib.rs:1454
             at /checkout/src/libcore/ops/function.rs:223
             at /checkout/src/liballoc/boxed.rs:640
  21: __rust_maybe_catch_panic
             at libpanic_unwind/lib.rs:106
thread 'raftstore_cases::test_prevote::test_prevote_reboot_minority_followers' panicked at 'called `Result::unwrap()` on an `Err` value: "PoisonError { inner: .. }"', libcore/result.rs:945:5
stack backtrace:
   0:     0x55dfffcb287e - std::sys::unix::backtrace::tracing::imp::unwind_backtrace::h53bc450f4e05b019
                               at libstd/sys/unix/backtrace/tracing/gcc_s.rs:49
   1:     0x55dfffcbba66 - std::sys_common::backtrace::print::h04a0ea06a78a82c5
                               at libstd/sys_common/backtrace.rs:71
                               at libstd/sys_common/backtrace.rs:59
   2:     0x55dfffcb6b0d - std::panicking::default_hook::{{closure}}::h13c5ed342ec5a2b6
                               at libstd/panicking.rs:211
   3:     0x55dfffcb6880 - std::panicking::default_hook::h72a62d53239218b1
                               at libstd/panicking.rs:227
   4:     0x55dfffcb717c - std::panicking::rust_panic_with_hook::h60a37fb79e459606
                               at libstd/panicking.rs:475
   5:     0x55dfffcb6d79 - std::panicking::continue_panic_fmt::hb542a58a3d52cb5c
                               at libstd/panicking.rs:390
   6:     0x55dfffcb6c75 - rust_begin_unwind
                               at libstd/panicking.rs:325
   7:     0x55dfffcffe7b - core::panicking::panic_fmt::h68c841fe431ac371
                               at libcore/panicking.rs:77
   8:     0x55dffd0548e2 - core::result::unwrap_failed::h8aec6539ee478311
                               at /checkout/src/libcore/macros.rs:26
   9:     0x55dffd05093e - <core::result::Result<T, E>>::unwrap::hec671cc8f2d83d35
                               at /checkout/src/libcore/result.rs:782
  10:     0x55dffd062f11 - <std::sync::rwlock::RwLock<T> as tikv::util::HandyRwLock<T>>::wl::h51119d372e9c3166
                               at /home/circleci/tikv-ci/src/util/mod.rs:138
  11:     0x55dffd0e1be4 - <test_raftstore::cluster::Cluster<T>>::stop_node::h748ce577a7dbdbf0
                               at /home/circleci/tikv-ci/components/test_raftstore/cluster.rs:205
  12:     0x55dffd0e1628 - <test_raftstore::cluster::Cluster<T>>::shutdown::h6bf917ee6d4685b5
                               at /home/circleci/tikv-ci/components/test_raftstore/cluster.rs:508
  13:     0x55dffd0e1e1d - <test_raftstore::cluster::Cluster<T> as core::ops::drop::Drop>::drop::ha4a9fb9b4fdff1c0
                               at /home/circleci/tikv-ci/components/test_raftstore/cluster.rs:967
  14:     0x55dffcbde8c0 - core::ptr::drop_in_place::h628fb28a860ccc5c
                               at /checkout/src/libcore/ptr.rs:59
  15:     0x55dffcedf96b - integrations::raftstore_cases::test_prevote::test_prevote_reboot_minority_followers::hcf4f47f4aa17b4ce
                               at tests/raftstore_cases/test_prevote.rs:219
  16:     0x55dffcc2b179 - integrations::__test::TESTS::{{closure}}::h60a009302172cd4a
                               at tests/raftstore_cases/test_prevote.rs:209
  17:     0x55dffcbc7bad - core::ops::function::FnOnce::call_once::hce773776a4142d84
                               at /checkout/src/libcore/ops/function.rs:223
  18:     0x55dffe8b7e0e - <F as alloc::boxed::FnBox<A>>::call_box::h1791e880f028d3d5
                               at libtest/lib.rs:1454
                               at /checkout/src/libcore/ops/function.rs:223
                               at /checkout/src/liballoc/boxed.rs:640
  19:     0x55dfffcc6e19 - __rust_maybe_catch_panic
                               at libpanic_unwind/lib.rs:106
  20:     0x55dffe8d913f - std::sys_common::backtrace::__rust_begin_short_backtrace::hc0f0325942c00b52
                               at /checkout/src/libstd/panicking.rs:289
                               at /checkout/src/libstd/panic.rs:392
                               at libtest/lib.rs:1409
                               at /checkout/src/libstd/sys_common/backtrace.rs:136
  21:     0x55dffe8d9b74 - std::panicking::try::do_call::h753b561d689539bb
                               at /checkout/src/libstd/thread/mod.rs:409
                               at /checkout/src/libstd/panic.rs:313
                               at /checkout/src/libstd/panicking.rs:310
  22:     0x55dfffcc6e19 - __rust_maybe_catch_panic
                               at libpanic_unwind/lib.rs:106
  23:     0x55dffe8cd646 - <F as alloc::boxed::FnBox<A>>::call_box::h9e1f0ce96f498f8a
                               at /checkout/src/libstd/panicking.rs:289
                               at /checkout/src/libstd/panic.rs:392
                               at /checkout/src/libstd/thread/mod.rs:408
                               at /checkout/src/liballoc/boxed.rs:640
  24:     0x55dfffca6d8a - std::sys_common::thread::start_thread::hbdf390fd59cb95cd
                               at /checkout/src/liballoc/boxed.rs:650
                               at libstd/sys_common/thread.rs:24
  25:     0x55dfffca4ff5 - std::sys::unix::thread::Thread::new::thread_start::h96884314b12ba24c
                               at libstd/sys/unix/thread.rs:90
  26:     0x2abff5f69183 - start_thread
  27:     0x2abff6492ffc - clone
  28:                0x0 - <unknown>
thread panicked while panicking. aborting.
error: process didn't exit successfully: `/home/circleci/tikv-ci/target/debug/deps/integrations-c6ec0543a63690c2 --nocapture` (signal: 4, SIGILL: illegal instruction)
make[1]: *** [test] Error 101
make[1]: Leaving directory `/home/circleci/tikv-ci'
find fail cases: test_prevote_reboot_minority_followers
@huachaohuang

This comment has been minimized.

Copy link
Contributor

huachaohuang commented Sep 20, 2018

@colinback Thanks, we will take a look at the tipb problem.

@huachaohuang

This comment has been minimized.

Copy link
Contributor

huachaohuang commented Sep 20, 2018

@colinback You can just remove the set_encode_type() after updating to the new tipb.
For the CI tests, I think it is not your fault, you can create an issue for the failed tests an rerun the test, thanks.

@huachaohuang

This comment has been minimized.

Copy link
Contributor

huachaohuang commented Sep 21, 2018

@colinback

This comment has been minimized.

Copy link
Contributor Author

colinback commented Sep 23, 2018

@huachaohuang After I remove set_encode_type() other errors occurs. Since it's not a single line change, I guess it should be fix in another pr that match code change of tipb PR 96.

  --> src/coprocessor/dag/dag.rs:18:39
   |
18 | use tipb::select::{Chunk, DAGRequest, EncodeType, SelectResponse, StreamResponse};
   |                                       ^^^^^^^^^^
   |
   = note: #[warn(unused_imports)] on by default

error[E0004]: non-exhaustive patterns: `JsonValidSig`, `JsonContainsSig`, `JsonArrayAppendSig` and 11 more not covered
  --> src/coprocessor/dag/expr/scalar_function.rs:26:42
   |
26 |         let (min_args, max_args) = match sig {
   |                                          ^^^ patterns `JsonValidSig`, `JsonContainsSig`, `JsonArrayAppendSig` and 11 more not covered

error: aborting due to previous error
@colinback

This comment has been minimized.

Copy link
Contributor Author

colinback commented Sep 23, 2018

I created issue #3630 about the ci-test.

row: &'a [Datum],
) -> Result<Option<Cow<'a, Decimal>>> {
let x = try_opt!(self.children[0].eval_decimal(ctx, row));
let d = try_opt!(self.children[1].eval_int(ctx, row));

This comment has been minimized.

Copy link
@DorianZheng

DorianZheng Sep 25, 2018

Member

@colinback Could you use some meaningful variable name? such as s/x/decimal and s/d/integer

This comment has been minimized.

Copy link
@colinback

colinback Sep 26, 2018

Author Contributor

@DorianZheng Oh, sure I will replace with some other name. Currently I just follow arguments in MySQL document.

https://dev.mysql.com/doc/refman/5.7/en/mathematical-functions.html#function_round

ROUND(X), ROUND(X,D)

Rounds the argument X to D decimal places. The rounding algorithm depends on the data type of X. D defaults to 0 if not specified. D can be negative to cause D digits left of the decimal point of the value X to become zero.

@huachaohuang

This comment has been minimized.

Copy link
Contributor

huachaohuang commented Sep 26, 2018

@colinback colinback force-pushed the colinback:Roundxxx_func branch from 9a8117a to c3718d4 Sep 27, 2018

coprocessor/dag/expr: add builtin round function(RoundDec, RoundReal,…
… RoundWithFracxxx)

Signed-off-by: shizy <shizy@cn.ibm.com>

@colinback colinback force-pushed the colinback:Roundxxx_func branch from c3718d4 to 0f8aab1 Sep 27, 2018

@colinback

This comment has been minimized.

Copy link
Contributor Author

colinback commented Sep 27, 2018

@DorianZheng I've renamed variable and fix conflict of master base.

@DorianZheng
Copy link
Member

DorianZheng left a comment

LGTM

@DorianZheng

This comment has been minimized.

Copy link
Member

DorianZheng commented Sep 27, 2018

@spongedu PTAL

@solotzg
Copy link
Contributor

solotzg left a comment

LGTM

@DorianZheng

This comment has been minimized.

Copy link
Member

DorianZheng commented Sep 29, 2018

@colinback Thank you for the PR. I will merge it as soon as the test pass.

@DorianZheng DorianZheng merged commit e28fd32 into tikv:master Sep 29, 2018

3 checks passed

DCO All commits are signed off!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
jenkins-ci-tikv/build Jenkins job succeeded.
Details

@AndreMouche AndreMouche referenced this pull request Dec 6, 2018

Open

coprocessor/expression: push down scalar functions #3275

333 of 469 tasks complete
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.