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

feat: add mysql engine to cli #228

Merged
merged 11 commits into from
Sep 9, 2024
Merged

Conversation

zjregee
Copy link
Contributor

@zjregee zjregee commented Jul 7, 2024

related: #120

@xxchan
Copy link
Member

xxchan commented Jul 29, 2024

Sorry, I missed the PR. Will review soon.

@zjregee
Copy link
Contributor Author

zjregee commented Sep 1, 2024

Hi, @xxchan, @BugenZhao. This PR is ready to review.

sqllogictest-engines/src/mysql.rs Outdated Show resolved Hide resolved
sqllogictest-engines/src/mysql.rs Outdated Show resolved Hide resolved
sqllogictest-engines/src/mysql.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@BugenZhao BugenZhao left a comment

Choose a reason for hiding this comment

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

Rest LGTM!

let value_str = match value {
Value::Bytes(bytes) => match String::from_utf8(bytes) {
Ok(x) => x,
Err(_) => "NULL".to_string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this reachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this shouldn't be reachable.

Ok(x) => x,
Err(_) => "NULL".to_string(),
},
_ => "NULL".to_string(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we explicitly assert other variants to be unreachable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The NULL judgment logic has been updated and is now clearer.

output.push(row_vec);
}
if output.is_empty() {
Ok(DBOutput::StatementComplete(0))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there anyway we can retrieve the affected row count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be obtained through conn.affected_rows, updated.

Signed-off-by: zjregee <[email protected]>
Signed-off-by: zjregee <[email protected]>
Signed-off-by: zjregee <[email protected]>
Signed-off-by: zjregee <[email protected]>
Signed-off-by: zjregee <[email protected]>
Signed-off-by: zjregee <[email protected]>
Signed-off-by: zjregee <[email protected]>
@@ -44,7 +44,7 @@ You can find more options in `sqllogictest --help` .

> **Note**
>
> Currently only postgres is supported in the CLI tool.
> Currently only postgres and mysql are supported in the CLI tool.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In addition, the description of the cli has been modified here.

@zjregee
Copy link
Contributor Author

zjregee commented Sep 6, 2024

It looks like there is an issue with semver ci that is not related to this PR.

Signed-off-by: xxchan <[email protected]>
Copy link
Member

@xxchan xxchan left a comment

Choose a reason for hiding this comment

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

Thank you for the PR!

@xxchan xxchan enabled auto-merge (squash) September 9, 2024 02:41
@xxchan xxchan merged commit a6bbfdb into risinglightdb:main Sep 9, 2024
3 of 4 checks passed
@xxchan
Copy link
Member

xxchan commented Sep 9, 2024

published to crates.io and github release

@zjregee zjregee deleted the add-mysql-engine branch September 9, 2024 03:51
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.

3 participants