prepared_statement: move parameters #2433
Conversation
Riolku
force-pushed
the
move-prepared-statement-parameters
branch
from
4900565
to
54cd4b1
Compare
Codecov ReportAttention:
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. |
Riolku
force-pushed
the
move-prepared-statement-parameters
branch
from
54cd4b1
to
d956f01
Compare
Riolku
force-pushed
the
move-prepared-statement-parameters
branch
from
d956f01
to
8ed0324
Compare
|
Should I add a test for the untested exception in the nodejs api? |
Up to you. I thought it should be KU_UNREACHABLE though |
| @@ -359,7 +359,6 @@ jobs: | |||
| run: | | |||
| ulimit -n 10240 | |||
| source /Users/runner/.cargo/env | |||
| cargo update -p cc --precise '1.0.83' | |||
There was a problem hiding this comment.
I thought this was required? Could @benjaminwinger comment?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see. Sorry I did not read it carefully.
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
force-pushed
the
move-prepared-statement-parameters
branch
from
8ed0324
to
63baff5
Compare
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. |
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
ccon MacOS.