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 nested value iteration #1845

Merged
merged 4 commits into from
Jul 23, 2023
Merged

Fix nested value iteration #1845

merged 4 commits into from
Jul 23, 2023

Conversation

andyfengHKU
Copy link
Contributor

@andyfengHKU andyfengHKU commented Jul 21, 2023

This PR fixes the performance issue of iterating nested data types. Now we avoid creating children value objects at iteration time (LIST still have to create since the size cannot be known before iteration, but we make sure it never resize to smaller size so the performance issue should be amortized.)

Meanwhile I always remove used APIs related to nodeVal and relVal. And remove all APIs that are triggering copy constructor from Value class.

TODOs

  • propagate change to on Rust side
  • propagate change to C, Java and NodeJS

src/common/types/value.cpp Show resolved Hide resolved
src/main/connection.cpp Show resolved Hide resolved
src/include/common/types/value.h Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Jul 23, 2023

Codecov Report

Patch coverage: 83.51% and project coverage change: +0.09 🎉

Comparison is base (d6abf42) 91.23% compared to head (72b0217) 91.32%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1845      +/-   ##
==========================================
+ Coverage   91.23%   91.32%   +0.09%     
==========================================
  Files         796      796              
  Lines       28983    28973      -10     
==========================================
+ Hits        26443    26461      +18     
+ Misses       2540     2512      -28     
Impacted Files Coverage Δ
src/common/arrow/arrow_row_batch.cpp 55.33% <0.00%> (+0.35%) ⬆️
src/include/common/types/value.h 100.00% <ø> (ø)
src/main/query_result.cpp 67.96% <0.00%> (ø)
src/c_api/value.cpp 86.95% <57.14%> (+1.05%) ⬆️
src/common/types/value.cpp 83.78% <88.99%> (+3.29%) ⬆️
src/expression_evaluator/literal_evaluator.cpp 89.74% <100.00%> (+0.55%) ⬆️
src/main/connection.cpp 93.20% <100.00%> (-0.03%) ⬇️
src/processor/result/factorized_table.cpp 95.64% <100.00%> (ø)
src/storage/storage_structure/in_mem_file.cpp 72.72% <100.00%> (+0.97%) ⬆️
tools/java_api/src/jni/kuzu_java.cpp 79.60% <100.00%> (+0.04%) ⬆️
... and 2 more

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@andyfengHKU andyfengHKU merged commit f6de8ea into master Jul 23, 2023
8 of 10 checks passed
@andyfengHKU andyfengHKU deleted the fix-nested-value-iteration branch July 23, 2023 20:05
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.

None yet

5 participants