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

Tikv conv function inconsistent results with tidb #51877

Closed
Tracked by #51876
yibin87 opened this issue Mar 19, 2024 · 13 comments · Fixed by #53502
Closed
Tracked by #51876

Tikv conv function inconsistent results with tidb #51877

yibin87 opened this issue Mar 19, 2024 · 13 comments · Fixed by #53502
Assignees
Labels
affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.1 This bug affects the 6.1.x(LTS) versions. affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. severity/major sig/execution SIG execution type/bug The issue is confirmed as a bug.

Comments

@yibin87
Copy link
Contributor

yibin87 commented Mar 19, 2024

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

set tidb_opt_projection_push_down = 'on';
create table bug15583(b BIT(8), n INT);
insert into bug15583 values(128, 128);
insert into bug15583 values(null, null);
insert into bug15583 values(0, 0);
insert into bug15583 values(255, 255);
select conv(b, 10, 2), conv(b + 0, 10, 2) from bug15583;

2. What did you expect to see? (Required)

conv(b, 10, 2) conv(b + 0, 10, 2)
10000000 10000000
NULL NULL
0 0
11111111 11111111

3. What did you see instead (Required)

conv(b, 10, 2) conv(b + 0, 10, 2)
0 10000000
NULL NULL
0 0
0 11111111

4. What is your TiDB version? (Required)

| Release Version: v7.4.0-alpha-2012-gfe01d11df6-dirty
Edition: Community
Git Commit Hash: fe01d11
Git Branch: div_increase_prec
UTC Build Time: 2024-03-15 08:10:07

@yibin87 yibin87 added the type/bug The issue is confirmed as a bug. label Mar 19, 2024
@yibin87
Copy link
Contributor Author

yibin87 commented Mar 19, 2024

/assign @yibin87

@yibin87
Copy link
Contributor Author

yibin87 commented Mar 19, 2024

Checked that tidb is consistent with mysql.

@aytrack
Copy link
Contributor

aytrack commented Mar 19, 2024

/label sig/execution
/label severity/major

Copy link

ti-chi-bot bot commented Mar 19, 2024

@aytrack: The label(s) sig/execution, severity/major cannot be applied. These labels are supported: fuzz/sqlancer, challenge-program, compatibility-breaker, first-time-contributor, contribution, good first issue, correctness, duplicate, proposal, security, ok-to-test, needs-ok-to-test, needs-more-info, needs-cherry-pick-release-5.4, needs-cherry-pick-release-6.1, needs-cherry-pick-release-6.5, needs-cherry-pick-release-7.1, needs-cherry-pick-release-7.5, needs-cherry-pick-release-8.0, affects-5.4, affects-6.1, affects-6.5, affects-7.1, affects-7.5, affects-8.0, may-affects-5.4, may-affects-6.1, may-affects-6.5, may-affects-7.1, may-affects-7.5, may-affects-8.0.

In response to this:

/label sig/execution
/label severity/major

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

@aytrack
Copy link
Contributor

aytrack commented Mar 19, 2024

/severity major
/sig execution

@ti-chi-bot ti-chi-bot added the affects-8.1 This bug affects the 8.1.x(LTS) versions. label Apr 9, 2024
@yibin87
Copy link
Contributor Author

yibin87 commented May 22, 2024

In latest tidb version, the predicate pushdown logic seems changed, and a little strange:

mysql> explain select conv(b, 10, 2) from bug15583;
+-------------------------+---------+-----------+----------------+-------------------------------------------------------------+
| id                      | estRows | task      | access object  | operator info                                               |
+-------------------------+---------+-----------+----------------+-------------------------------------------------------------+
| TableReader_8           | 4.00    | root      |                | data:Projection_4                                           |
| └─Projection_4          | 4.00    | cop[tikv] |                | conv(cast(test.bug15583.b, var_string(1)), 10, 2)->Column#4 |
|   └─TableFullScan_7     | 4.00    | cop[tikv] | table:bug15583 | keep order:false, stats:pseudo                              |
+-------------------------+---------+-----------+----------------+-------------------------------------------------------------+
3 rows in set (0.00 sec)

mysql> explain select conv(b + 0, 10, 2) from bug15583;
+-------------------------+---------+-----------+----------------+-----------------------------------------------------------------------+
| id                      | estRows | task      | access object  | operator info                                                         |
+-------------------------+---------+-----------+----------------+-----------------------------------------------------------------------+
| TableReader_8           | 4.00    | root      |                | data:Projection_4                                                     |
| └─Projection_4          | 4.00    | cop[tikv] |                | conv(cast(plus(test.bug15583.b, 0), var_string(20)), 10, 2)->Column#4 |
|   └─TableFullScan_7     | 4.00    | cop[tikv] | table:bug15583 | keep order:false, stats:pseudo                                        |
+-------------------------+---------+-----------+----------------+-----------------------------------------------------------------------+
3 rows in set (0.00 sec)

mysql> explain select conv(b, 10, 2), conv(b + 0, 10, 2) from bug15583;
+-------------------------+---------+-----------+----------------+------------------------------------------------------------------------------------------------------------------------------------+
| id                      | estRows | task      | access object  | operator info                                                                                                                      |
+-------------------------+---------+-----------+----------------+------------------------------------------------------------------------------------------------------------------------------------+
| Projection_3            | 4.00    | root      |                | conv(cast(test.bug15583.b, var_string(1)), 10, 2)->Column#4, conv(cast(plus(test.bug15583.b, 0), var_string(20)), 10, 2)->Column#5 |
| └─TableReader_6         | 4.00    | root      |                | data:TableFullScan_5                                                                                                               |
|   └─TableFullScan_5     | 4.00    | cop[tikv] | table:bug15583 | keep order:false, stats:pseudo                                                                                                     |
+-------------------------+---------+-----------+----------------+------------------------------------------------------------------------------------------------------------------------------------+
3 rows in set (0.00 sec)

@yibin87
Copy link
Contributor Author

yibin87 commented May 23, 2024

For conv function implementation, it handles the case that its argument is cast(hybrid_type as string) function specially:

case *ScalarFunction:
if x.FuncName.L == ast.Cast {
arg0 := x.GetArgs()[0]
if arg0.GetType().Hybrid() || IsBinaryLiteral(arg0) {
str, isNull, err = arg0.EvalString(ctx, row)
if isNull || err != nil {
return str, isNull, err
}
d := types.NewStringDatum(str)
str = d.GetBinaryLiteral().ToBitLiteralString(true)
}
}
}

It first converts hybrid_type to bit string literal like: b'01010101, then use internal conv(bit_literal_string, 2, from_base) to get temporary result, and finally conv(tem_result, from_base, to_base)
For TiKV, its expression calculation mechanism doesn't support this "trick", thus we'd better disallow such case to be pushed down to TiKV.
For TiFlash, hybrid_type within Functions are not allowed to be pushed down, thus, nothing to be worried.

@yibin87
Copy link
Contributor Author

yibin87 commented May 23, 2024

/label affects-7.5

@ti-chi-bot ti-chi-bot bot added affects-7.5 This bug affects the 7.5.x(LTS) versions. and removed may-affects-7.5 labels May 23, 2024
@yibin87
Copy link
Contributor Author

yibin87 commented May 23, 2024

/label affects-7.1

@ti-chi-bot ti-chi-bot bot added affects-7.1 This bug affects the 7.1.x(LTS) versions. and removed may-affects-7.1 labels May 23, 2024
@yibin87
Copy link
Contributor Author

yibin87 commented May 23, 2024

/label affects-6.5

@ti-chi-bot ti-chi-bot bot added affects-6.5 This bug affects the 6.5.x(LTS) versions. and removed may-affects-6.5 labels May 23, 2024
@yibin87
Copy link
Contributor Author

yibin87 commented May 23, 2024

/label affects-6.1

@ti-chi-bot ti-chi-bot bot added the affects-6.1 This bug affects the 6.1.x(LTS) versions. label May 23, 2024
@yibin87
Copy link
Contributor Author

yibin87 commented May 23, 2024

/label affects-5.4

@ti-chi-bot ti-chi-bot bot added affects-5.4 This bug affects the 5.4.x(LTS) versions. and removed may-affects-5.4 This bug maybe affects 5.4.x versions. labels May 23, 2024
@yibin87
Copy link
Contributor Author

yibin87 commented May 23, 2024

In latest tidb version, the predicate pushdown logic seems changed, and a little strange:

mysql> explain select conv(b, 10, 2) from bug15583;
+-------------------------+---------+-----------+----------------+-------------------------------------------------------------+
| id                      | estRows | task      | access object  | operator info                                               |
+-------------------------+---------+-----------+----------------+-------------------------------------------------------------+
| TableReader_8           | 4.00    | root      |                | data:Projection_4                                           |
| └─Projection_4          | 4.00    | cop[tikv] |                | conv(cast(test.bug15583.b, var_string(1)), 10, 2)->Column#4 |
|   └─TableFullScan_7     | 4.00    | cop[tikv] | table:bug15583 | keep order:false, stats:pseudo                              |
+-------------------------+---------+-----------+----------------+-------------------------------------------------------------+
3 rows in set (0.00 sec)

mysql> explain select conv(b + 0, 10, 2) from bug15583;
+-------------------------+---------+-----------+----------------+-----------------------------------------------------------------------+
| id                      | estRows | task      | access object  | operator info                                                         |
+-------------------------+---------+-----------+----------------+-----------------------------------------------------------------------+
| TableReader_8           | 4.00    | root      |                | data:Projection_4                                                     |
| └─Projection_4          | 4.00    | cop[tikv] |                | conv(cast(plus(test.bug15583.b, 0), var_string(20)), 10, 2)->Column#4 |
|   └─TableFullScan_7     | 4.00    | cop[tikv] | table:bug15583 | keep order:false, stats:pseudo                                        |
+-------------------------+---------+-----------+----------------+-----------------------------------------------------------------------+
3 rows in set (0.00 sec)

mysql> explain select conv(b, 10, 2), conv(b + 0, 10, 2) from bug15583;
+-------------------------+---------+-----------+----------------+------------------------------------------------------------------------------------------------------------------------------------+
| id                      | estRows | task      | access object  | operator info                                                                                                                      |
+-------------------------+---------+-----------+----------------+------------------------------------------------------------------------------------------------------------------------------------+
| Projection_3            | 4.00    | root      |                | conv(cast(test.bug15583.b, var_string(1)), 10, 2)->Column#4, conv(cast(plus(test.bug15583.b, 0), var_string(20)), 10, 2)->Column#5 |
| └─TableReader_6         | 4.00    | root      |                | data:TableFullScan_5                                                                                                               |
|   └─TableFullScan_5     | 4.00    | cop[tikv] | table:bug15583 | keep order:false, stats:pseudo                                                                                                     |
+-------------------------+---------+-----------+----------------+------------------------------------------------------------------------------------------------------------------------------------+
3 rows in set (0.00 sec)

Currently, the cost model v2 estimated the pushdown and not-pushdown plan's cost, and choose the not-pushdown plan. The main difference between the two plans is: the network traffic, pushdown-plan will occupy more network bandwith.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
affects-5.4 This bug affects the 5.4.x(LTS) versions. affects-6.1 This bug affects the 6.1.x(LTS) versions. affects-6.5 This bug affects the 6.5.x(LTS) versions. affects-7.1 This bug affects the 7.1.x(LTS) versions. affects-7.5 This bug affects the 7.5.x(LTS) versions. affects-8.1 This bug affects the 8.1.x(LTS) versions. severity/major sig/execution SIG execution type/bug The issue is confirmed as a bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants