-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
expression: handle empty input and improve compatibility for format
#8797
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rest lgtm
- change format with locale - modify tests - handle null in format with locale
expression/builtin_string.go
Outdated
// evalString evals FORMAT(X,D). | ||
// See https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_format | ||
func (b *builtinFormatSig) evalString(row chunk.Row) (string, bool, error) { | ||
x, isNull, err := b.args[0].EvalString(b.ctx, row) | ||
x, isNull, err := b.args[0].EvalReal(b.ctx, row) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument types are all ETString
: https://github.com/pingcap/tidb/pull/8797/files#diff-314e997a9df9b116e8f0aad4149df468R2920. We should not change EvalString()
to EvalReal()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the output of MySQL:
mysql> select format("1a","2a");
+-------------------+
| format("1a","2a") |
+-------------------+
| 1.00 |
+-------------------+
1 row in set, 2 warnings (0.00 sec)
mysql> show warnings;
+---------+------+-----------------------------------------+
| Level | Code | Message |
+---------+------+-----------------------------------------+
| Warning | 1292 | Truncated incorrect INTEGER value: '2a' |
| Warning | 1292 | Truncated incorrect DOUBLE value: '1a' |
+---------+------+-----------------------------------------+
2 rows in set (0.00 sec)
Seems MySQL believes the type of parameters to be Double
and Integer
, but it is not reflected in https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_format.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In MySQL source code, for function format(X, D)
, X
is evaluated as decimal or double type, while D
is evaluated as integer type.
dec= (int) args[1]->val_int();
if (args[0]->result_type() == DECIMAL_RESULT ||
args[0]->result_type() == INT_RESULT)
{
res= args[0]->val_decimal(&dec_val);
}
else
{
double nr= args[0]->val_real();
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems we should modify https://github.com/pingcap/tidb/pull/8797/files#diff-314e997a9df9b116e8f0aad4149df468R2920 to change the argument type.
@lamxTyler @XuHuaiyu PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/run-all-tests |
/run-common-test |
1 similar comment
/run-common-test |
What problem does this PR solve?
Before this PR, in TiDB:
while in MySQL 5.7.10:
What is changed and how it works?
format
properly to avoid panic;30
is not documented in https://dev.mysql.com/doc/refman/5.7/en/string-functions.html#function_format, we get it from MySQL source code:select format(1, 4, null)
compatible with MySQL.Check List
Tests
Code changes
N/A
Side effects
N/A
Related changes
format
:format
incompatible with MySQL for round up #8796This change is