-
Notifications
You must be signed in to change notification settings - Fork 206
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
fix: remove unnecessary type conversion in MysqlWorker #606
Conversation
Codecov Report
@@ Coverage Diff @@
## main #606 +/- ##
==========================================
+ Coverage 65.97% 66.01% +0.04%
==========================================
Files 285 285
Lines 44341 44358 +17
==========================================
+ Hits 29256 29285 +29
+ Misses 15085 15073 -12
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
@Huachao Thanks for your contribution. Our testcases doesn't cover MySQL protocols now, would you mind add one? or you can just test by yourself, and paste what you tested here. |
@jiacai2050 currently I test it manually with following sql statements.
CREATE TABLE `test` (`name` string TAG, `value` double NOT NULL, `t` timestamp NOT NULL, TIMESTAMP KEY(t)) ENGINE=Analytic with (enable_ttl='false');
mysql> select * from test;
Query OK, 0 rows affected (0.05 sec)
insert into test (t, name, value) VALUES (1651737067000, 'ceresdb', 100);
mysql> select * from test;
+----------------------+---------------+---------+-------+
| tsid | t | name | value |
+----------------------+---------------+---------+-------+
| 12128845460635970325 | 1651737067000 | ceresdb | 100 |
+----------------------+---------------+---------+-------+
1 row in set (0.00 sec)
insert into test (t, name, value) VALUES (1651737068000, 'ceresdb', 200);
mysql> select * from test;
+----------------------+---------------+---------+-------+
| tsid | t | name | value |
+----------------------+---------------+---------+-------+
| 12128845460635970325 | 1651737067000 | ceresdb | 100 |
| 12128845460635970325 | 1651737068000 | ceresdb | 200 |
+----------------------+---------------+---------+-------+
2 rows in set (0.00 sec) |
The tests look good 👍 We can merge this PR after you fix column name extraction using only first RecordBatch. |
@Huachao I have push minor updates to your branch, will merge when CI pass. |
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
Thanks again 🚀 |
* fix:remove unnecessary type conversion in MysqlWorker (apache#596) * fix format error * fix format error * resolve comments * resolve comments * refactor conversion --------- Co-authored-by: jiacai2050 <dev@liujiacai.net>
Which issue does this PR close?
Closes #596
Rationale for this change
What changes are included in this PR?
Remove
Output
toResponse
convensionAre there any user-facing changes?
No
How does this change test
CI