expression:process flag overflow_as_warning #2901
Conversation
AndreMouche
changed the title
| @@ -101,61 +101,72 @@ pub fn bytes_to_int_without_context(bytes: &[u8]) -> Result<i64> { | |||
| // trim | |||
There was a problem hiding this comment.
Remove comment "Note that it does NOT handle overflow.".
src/coprocessor/codec/convert.rs
Outdated
| vec![b"9223372036854775809", b"-9223372036854775810"]; | ||
| for bs in invalid_cases { | ||
| let t = super::bytes_to_int_without_context(bs); | ||
| if !t.is_err() { |
There was a problem hiding this comment.
could you also check whether it is indeed an overflow error?
src/coprocessor/codec/convert.rs
Outdated
| let invalid_cases: Vec<&'static [u8]> = vec![b"18446744073709551616"]; | ||
| for bs in invalid_cases { | ||
| let t = super::bytes_to_uint_without_context(bs); | ||
| if !t.is_err() { |
src/coprocessor/dag/expr/err.rs
Outdated
| } | ||
|
|
||
| impl Error { | ||
| pub fn gen_overflow(data: &str, range: String) -> Error { |
There was a problem hiding this comment.
I think you can move these two parameters to the definition in quick_error so that Error::Overflow(data, range) will act as your Error::gen_overflow(data, range) directly.
src/coprocessor/dag/expr/err.rs
Outdated
| Error::Overflow(msg) | ||
| } | ||
|
|
||
| pub fn gen_truncated_wrong_val(data_type: &str, val: String) -> Error { |
| Error::Eval(msg.into(), ERR_UNKNOWN) | ||
| } | ||
|
|
||
| pub fn unknown_timezone(tz: i64) -> Error { |
|
As described previously, |
|
address comments @breeswish |
src/coprocessor/dag/expr/ctx.rs
Outdated
| @@ -164,6 +177,36 @@ impl EvalContext { | |||
| Err(err) | |||
| } | |||
|
|
|||
| /// handle_over_flow treats ErrOverflow as warnings or returns the error | |||
| /// based on the cfg.handle_over_flow state. | |||
| pub fn handle_over_flow(&mut self, err: Error) -> Result<()> { | |||
There was a problem hiding this comment.
why handle_over_flow instead of handle_overflow?
src/coprocessor/dag/expr/ctx.rs
Outdated
| orig_err: Error, | ||
| negitive: bool, | ||
| ) -> Result<i64> { | ||
| if !self.cfg.in_select_stmt | !self.cfg.overflow_as_warning { |
src/coprocessor/dag/expr/err.rs
Outdated
| } | ||
|
|
||
| impl Error { | ||
| pub fn overflow(data: &str, range: String) -> Error { |
There was a problem hiding this comment.
I prefer range: &str to keep in same style with data.
And I think it should be expr instead of range.
|
rest LGTM. |
|
@AndreMouche ci failed. |
|
It's not caused by current PR @zz-jason |
|
/run-integration-tests |
|
LGTM |
|
LGTM. |
|
/run-integration-tests |
|
LGTM |
dev
test
@hicqu @breeswish @XuHuaiyu PTAL