ISSUE:3175 fix try_from_literal

[dust1]

I hereby agree to the terms of the CLA available at: https://databend.rs/policies/cla/

Summary

I found the problem is in the try_from_literal function of DataValue, some u64 data will be lost when converting str to i64

Changelog

• Bug Fix

Related Issues

Fixes #3175

Test Plan

Unit Tests

Stateless Tests

[databend-bot]

Thanks for the contribution! I have applied any labels matching special text in your PR Changelog.

Please review the labels and make any necessary changes.

[codecov-commenter]

Codecov Report

Merging #3197 (2742763) into main (a40722d) will decrease coverage by 0%. The diff coverage is 94%.

@@          Coverage Diff          @@
##            main   #3197   +/-   ##
=====================================
- Coverage     65%     65%   -1%     
=====================================
  Files        705     705           
  Lines      38273   38277    +4     
=====================================
- Hits       24928   24927    -1     
- Misses     13345   13350    +5     

Impacted Files	Coverage Δ
common/datavalues/src/data_value_ops.rs	60% <94%> (+18%)	:arrow_up:
cli/src/error.rs	29% <0%> (-4%)	:arrow_down:
common/management/src/cluster/cluster_mgr.rs	78% <0%> (-3%)	:arrow_down:
metasrv/src/store/meta_raft_store.rs	76% <0%> (-3%)	:arrow_down:
common/datavalues/src/data_value.rs	42% <0%> (+<1%)	:arrow_up:
...pelines/transforms/transform_aggregator_partial.rs	87% <0%> (+1%)	:arrow_up:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update a40722d...2742763. Read the comment docs.

[dust1]

/review @sundy-li

[databend-bot]

Take the reviewer to sundy-li

[sundy-li]

maybe we can add a stateless test to cover the issue #3175

[dust1]

ok.

[dust1]

hello @sundy-li , I have a question. when i exec select -1;, I find the data type is Int16 in PlanNode. I have no database work experience, so I can’t be sure if this is a bug, does this mean that databend will expand the range upward when it gets the type of a signed integer?

Expr has extracted ‘-’, so the parameter to DataValue:: try_from_literal is unsigned, the result type was UInt8, and then I found code about display PlanNode to find why, i found this: https://github.com/datafuselabs/databend/blob/335293a66f62d581fba551766dd9ccdf69df165f/common/datavalues/src/types/data_type_coercion.rs#L286-L293

[sundy-li]

select -1 returns Int16

This is a bug, I think it's related to sqlparser, - is extracted as operator, we may need to modify the sqlparser to let -1 be represented as literal value.

[dust1]

To complete this work, do I need to submit pr in datafuse-extras/sqlparser-rs ?

[sundy-li]

To complete this work, do I need to submit pr in datafuse-extras/sqlparser-rs ?

That's ok, it's a small library. You can hold it.

[dust1]

ok

[dust1]

hello @sundy-li , I read the code related to datafuse-extras/sqlparser-rs and looked up articles related to sqlparser. I found that if the value in Number(String, bool) is used as a signed integer to generate Expr like -1, I can’t control the consequences of the modification with my current knowledge, this processing method seems to be different from the existing sqlparser algorithm. Even if -1 is used as the literal value, how to deal with op:Minus also seems to have big problems.

I recheck some stateless test again , The sql result works fine when use unsigned integers:

select toTypeName(a) from (select 255 as a);                                => UInt8
select toTypeName(a) from (select 65535 as a);                              => UInt16
select toTypeName(a) from (select 4294967295 as a);                         => UInt32
select toTypeName(a) from (select 18446744073709551615 as a);               => UInt64
select toTypeName(a) from (select 1844674407370955161522 as a);             => Float64

This problem may be limited to ArithmeticFunction, maybe we only need to fix part of the code related to Minus

[sundy-li]

Related codes:

            Token::Mul => Ok(Expr::Wildcard),
            tok @ Token::Minus | tok @ Token::Plus => {
                let op = if tok == Token::Plus {
                    UnaryOperator::Plus
                } else {
                    UnaryOperator::Minus
                };
                Ok(Expr::UnaryOp {
                    op,
                    expr: Box::new(self.parse_subexpr(Self::PLUS_MINUS_PREC)?),
                })
            }

[vercel[bot]]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/databend/databend/FGhMpqWAGcMvCa3cwd18b7nzEG5U
✅ Preview: https://databend-git-fork-dust1-dust1-issue3175-databend.vercel.app

[mergify[bot]]

This pull request has merge conflicts that must be resolved before it can be merged. @dust1 please rebase it 🙏

[sundy-li]

/LGTM, but there are conflicts to merge.

[databend-bot]

Wait for another reviewer approval

[dust1]

/LGTM, but there are conflicts to merge.

should i use git rebase to merge this pr? I haven't touched before

[databend-bot]

Wait for another reviewer approval

[sundy-li]

should i use git rebase to merge this pr? I haven't touched before

git pull --rebase origin main then fix the conflicts.

[databend-bot]

Wait for another reviewer approval

[BohuTANG]

@mergifyio update

[mergify[bot]]

update

✅ Branch has been successfully updated

[BohuTANG]

I guess the rebase has some problems here

[BohuTANG]

I guess the rebase has some problems here

It's OK now :)

[databend-bot]

Wait for another reviewer approval