coprocessor/dag/expr: add some un-pushed builtin UDF(Reverse, ReverseBinary) #3435
Conversation
Signed-off-by: spongedc <duchuangucas@live.com>
|
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
added
sig/coprocessor
Signed-off-by: spongedc <duchuangucas@live.com>
spongedu
dismissed stale reviews from breezewish, AndreMouche, and siddontang
via
c914a39
|
@breeswish conflicts resolved. PTAL ~ |
| ctx: &mut EvalContext, | ||
| row: &'a [Datum], | ||
| ) -> Result<Option<Cow<'a, [u8]>>> { | ||
| let s = try_opt!(self.children[0].eval_string(ctx, row)); |
There was a problem hiding this comment.
I think here we need to call eval_string_and_decode instead of call str::from_utf8 directly.
There was a problem hiding this comment.
@hicqu . I've fix this. PTAL. I made some modifications on eval_string_and_decode, disable the charset check and use a TODO comment instead because different charsets may be pushed down from TiDB and some builtins depends on it. UPPER, LOWER for example.
|
rest LGTM. |
Signed-off-by: spongedc <duchuangucas@live.com>
| if types::is_binary_str(self.children[0].get_tp()) { | ||
| return Ok(Some(s)); | ||
| return Ok(Some(Cow::Owned(s.to_string().into_bytes()))); |
There was a problem hiding this comment.
Maybe you can directly s.into_bytes().
There was a problem hiding this comment.
&str has no into_bytes method . We can either s.to_string().into_bytes() or s.as_bytes().to_vec(), I think they are equal....
Signed-off-by: spongedc <duchuangucas@live.com>
Signed-off-by: spongedc <duchuangucas@live.com>
src/coprocessor/dag/expr/mod.rs
Outdated
| return s.map(Some); | ||
| } | ||
| Err(box_err!("unsupported charset: {}", chrst)) | ||
| // TODO: Deal with more supported codec in the future |
| ]; | ||
| let mut ctx = EvalContext::default(); | ||
| for (arg, exp) in cases { | ||
| let op = scalar_func_expr(ScalarFuncSig::Reverse, &[datum_expr(arg)]); |
There was a problem hiding this comment.
I think we need to do op.mut_field_type().set_charset(...). Now only utf8 is supported, so set it to utf8 is ok.
There was a problem hiding this comment.
@hicqu In TiDB, for binary string types such as BINARY, VARBINARY and so on, the charset and collate is set to binary. These settings are pushed down to the kv layer. If we panic on non-utf8 charsets, we can't deal with these binary string types. For example, type BINARY has the type setting as follows:
for (input, exp) in cases {
let input = string_datum_expr_with_tp(
input,
VAR_STRING,
BINARY_FLAG,
-1,
CHARSET_BIN.to_owned(),
COLLATION_BIN_ID,
);and the implementation of builtin UPPER in TiDB is as follows:
func (b *builtinUpperSig) evalString(row chunk.Row) (d string, isNull bool, err error) {
d, isNull, err = b.args[0].EvalString(b.ctx, row)
if isNull || err != nil {
return d, isNull, errors.Trace(err)
}
if types.IsBinaryStr(b.args[0].GetType()) {
return d, false, nil
}
return strings.ToUpper(d), false, nil
}…val_string_and_decode' Signed-off-by: spongedc <duchuangucas@live.com>
|
@hicqu I've revert the changes on |
spongedu
changed the title
|
LGTM. :) |
Signed-off-by: spongedc <duchuangucas@live.com>
|
@hicqu @AndreMouche PTAL |
|
@hicqu @breeswish Please check and approve if everything goes well. |
|
@hicqu @breeswish PTAL |
|
LGTM. |
|
/run-unit-test |
…Binary) (tikv#3435) Signed-off-by: spongedc <duchuangucas@live.com>
What have you changed? (mandatory)
implement builtin function:
Reverse,ReverseBinaryincoprocessor.This is for #3275
What are the type of the changes? (mandatory)
Improvement
How has this PR been tested? (mandatory)
unittest
Does this PR affect documentation (docs/docs-cn) update? (mandatory)
Does this PR affect tidb-ansible update? (mandatory)
Refer to a related PR or issue link (optional)
#3275
Benchmark result if necessary (optional)
Add a few positive/negative examples (optional)