Skip to content

[jdbc-v2] Fixes in the PreparedStatementImpl class, the initial length of the parameters is incorrectly changed in the clearParameters method #2305

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

Merged
merged 5 commits into from
Apr 17, 2025

Conversation

CCweixiao
Copy link

Summary

Closes #2299

Checklist

Delete items not relevant to your PR:

  • Closes #
  • Unit and integration tests covering the common scenarios were added
  • A human-readable description of the changes was provided to include in CHANGELOG
  • For significant changes, documentation in https://github.com/ClickHouse/clickhouse-docs was updated with further explanations or tutorials

@CLAassistant
Copy link

CLAassistant commented Apr 14, 2025

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ chernser
❌ jielongping


jielongping seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@chernser
Copy link
Contributor

Good day, @CCweixiao!
Thank you for the contribution!
Would you please add at least one test to verify the problem is fixed?

Thanks!

jielongping added 2 commits April 17, 2025 09:57
…h of the parameters is incorrectly changed in the clearParameters method
@CCweixiao
Copy link
Author

CCweixiao commented Apr 17, 2025

Good day, @CCweixiao! Thank you for the contribution! Would you please add at least one test to verify the problem is fixed?

Thanks!

Thanks a lot for the code review, I've submitted a test case for the corresponding code fix, please help me review again. @chernser

}

@Override
String compileSql(String[] segments) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not a good practice - if someone change an original method then test will not be verifying a production code.
Please just make compileSql package private. This is ok because tests sometimes need to do so.

}
}

static class TestPreparedStatementImpl extends PreparedStatementImpl {
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove this class.

@chernser
Copy link
Contributor

@CCweixiao thanks!
I've reviewed - please see my comments.

@CCweixiao
Copy link
Author

@CCweixiao thanks! I've reviewed - please see my comments.

Your suggestion was taken and the test case modifications were completed @chernser

@mshustov mshustov requested a review from chernser April 17, 2025 07:03
@mzitnik
Copy link
Contributor

mzitnik commented Apr 17, 2025

@CCweixiao can you sign the CLA

@CCweixiao
Copy link
Author

@CCweixiao can you sign the CLA

image

@chernser chernser merged commit 6ecc277 into ClickHouse:main Apr 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants