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

prepared_statement: move parameters #2433

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

Riolku
Copy link
Contributor

@Riolku Riolku commented Nov 16, 2023

In the Java API, we had a bug where we take ownership of and free parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then performing a deep copy, which is nonsense. Instead, we should take a unique_ptr, since we need to copy the parameters to guarantee that they are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running any tests from ConnectionTest.java, which is why we didn't observe this bug. Additionally, the constructor of KuzuConnection uses an assertion, but assertions are disabled by default, which causes our tests to fail (and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been fixed, we can remove the version pinning of cc on MacOS.

@Riolku Riolku force-pushed the move-prepared-statement-parameters branch from 4900565 to 54cd4b1 Compare November 16, 2023 21:53
Copy link

codecov bot commented Nov 16, 2023

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (093e627) 90.54% compared to head (63baff5) 90.81%.
Report is 11 commits behind head on master.

Files Patch % Lines
tools/nodejs_api/src_cpp/node_util.cpp 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2433      +/-   ##
==========================================
+ Coverage   90.54%   90.81%   +0.26%     
==========================================
  Files        1020     1023       +3     
  Lines       36515    37024     +509     
==========================================
+ Hits        33064    33624     +560     
+ Misses       3451     3400      -51     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Riolku Riolku force-pushed the move-prepared-statement-parameters branch from 54cd4b1 to d956f01 Compare November 16, 2023 22:08
@Riolku Riolku force-pushed the move-prepared-statement-parameters branch from d956f01 to 8ed0324 Compare November 17, 2023 00:24
Copy link
Contributor

@andyfengHKU andyfengHKU left a comment

Choose a reason for hiding this comment

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

LGTM

tools/java_api/src/main/java/com/kuzudb/KuzuValue.java Outdated Show resolved Hide resolved
@Riolku
Copy link
Contributor Author

Riolku commented Nov 17, 2023

Should I add a test for the untested exception in the nodejs api?

@andyfengHKU
Copy link
Contributor

Should I add a test for the untested exception in the nodejs api?

Up to you. I thought it should be KU_UNREACHABLE though

Copy link
Member

@mewim mewim left a comment

Choose a reason for hiding this comment

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

API changes LGTM

@@ -359,7 +359,6 @@ jobs:
run: |
ulimit -n 10240
source /Users/runner/.cargo/env
cargo update -p cc --precise '1.0.83'
Copy link
Member

Choose a reason for hiding this comment

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

I thought this was required? Could @benjaminwinger comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of rust-lang/cc-rs#900 being fixed, this is no longer required. A note on this is also in the commit message for logging.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Sorry I did not read it carefully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries :)

In the Java API, we had a bug where we take ownership of and free
parameters passed into executeWithParams.

Inspecting the method itself, it was taking a shared_ptr, but then
performing a deep copy, which is nonsense. Instead, we should take a
unique_ptr, since we need to copy the parameters to guarantee that they
are not modified for the duration of the query.

This commit also fixes three other issues. First, the Java tests weren't running
any tests from ConnectionTest.java, which is why we didn't observe this
bug. Additionally, the constructor of KuzuConnection uses an assertion,
but assertions are disabled by default, which causes our tests to fail
(and if the assertion is skipped, we segfault).

Also, since rust-lang/cc-rs#900 has been
fixed, we can remove the version pinning of `cc` on MacOS.
@Riolku Riolku force-pushed the move-prepared-statement-parameters branch from 8ed0324 to 63baff5 Compare November 17, 2023 15:14
@Riolku
Copy link
Contributor Author

Riolku commented Nov 17, 2023

Should I add a test for the untested exception in the nodejs api?

Up to you. I thought it should be KU_UNREACHABLE though

Well since we have datatypes that we don't support in NodeJS it's not unreachable 😄 .

It should probably be marked UNREACHABLE in the future when we have better datatype support. I'm leaving it as is for now.

@Riolku Riolku merged commit b1cef8e into master Nov 17, 2023
11 of 12 checks passed
@Riolku Riolku deleted the move-prepared-statement-parameters branch November 17, 2023 15:46
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