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

fix: remove unnecessary type conversion in MysqlWorker #606

Merged
merged 6 commits into from
Feb 3, 2023

Conversation

Huachao
Copy link
Contributor

@Huachao Huachao commented Feb 2, 2023

Which issue does this PR close?

Closes #596

Rationale for this change

What changes are included in this PR?

Remove Output to Response convension

Are there any user-facing changes?

No

How does this change test

CI

@jiacai2050 jiacai2050 changed the title fix:remove unnecessary type conversion in MysqlWorker (#596) fix: remove unnecessary type conversion in MysqlWorker Feb 2, 2023
@jiacai2050 jiacai2050 self-requested a review February 2, 2023 09:54
@codecov-commenter
Copy link

codecov-commenter commented Feb 2, 2023

Codecov Report

Merging #606 (9c2f71b) into main (639c2bd) will increase coverage by 0.04%.
The diff coverage is 50.00%.

@@            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     
Impacted Files Coverage Δ
server/src/mysql/worker.rs 0.00% <0.00%> (ø)
server/src/mysql/writer.rs 56.54% <50.66%> (+13.21%) ⬆️
wal/src/rocks_impl/manager.rs 92.53% <0.00%> (-0.19%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@jiacai2050
Copy link
Contributor

jiacai2050 commented Feb 2, 2023

@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.

@Huachao
Copy link
Contributor Author

Huachao commented Feb 2, 2023

@jiacai2050 currently I test it manually with following sql statements.

  1. Create a test table
CREATE TABLE `test` (`name` string TAG, `value` double NOT NULL, `t` timestamp NOT NULL, TIMESTAMP KEY(t)) ENGINE=Analytic with (enable_ttl='false');
  1. Check the empty test table output of select query
mysql> select * from test;
Query OK, 0 rows affected (0.05 sec)
  1. Insert the first record
insert into test (t, name, value) VALUES (1651737067000, 'ceresdb', 100);
  1. Check the test table output of select query
mysql> select * from test;
+----------------------+---------------+---------+-------+
| tsid                 | t             | name    | value |
+----------------------+---------------+---------+-------+
| 12128845460635970325 | 1651737067000 | ceresdb |   100 |
+----------------------+---------------+---------+-------+
1 row in set (0.00 sec)
  1. Insert the second record
insert into test (t, name, value) VALUES (1651737068000, 'ceresdb', 200);
  1. Check the test table output of select query
mysql> select * from test;
+----------------------+---------------+---------+-------+
| tsid                 | t             | name    | value |
+----------------------+---------------+---------+-------+
| 12128845460635970325 | 1651737067000 | ceresdb |   100 |
| 12128845460635970325 | 1651737068000 | ceresdb |   200 |
+----------------------+---------------+---------+-------+
2 rows in set (0.00 sec)

@jiacai2050
Copy link
Contributor

The tests look good 👍

We can merge this PR after you fix column name extraction using only first RecordBatch.

@jiacai2050
Copy link
Contributor

@Huachao I have push minor updates to your branch, will merge when CI pass.

Copy link
Contributor

@jiacai2050 jiacai2050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jiacai2050
Copy link
Contributor

Thanks again 🚀

@jiacai2050 jiacai2050 merged commit 475dc6b into apache:main Feb 3, 2023
chunshao90 pushed a commit to chunshao90/ceresdb that referenced this pull request May 15, 2023
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove unnecessary type conversion in MysqlWorker
3 participants