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 Serial for node-groups #1886

Merged
merged 1 commit into from
Aug 11, 2023
Merged

Fix Serial for node-groups #1886

merged 1 commit into from
Aug 11, 2023

Conversation

aziz-mu
Copy link
Contributor

@aziz-mu aziz-mu commented Aug 3, 2023

This PR fixes COPY from CSV, Parquet, NPY for tables which include the SERIAL data type. Also added tests.

This is a continuation of this PR: #1863, rebase/merge was too complicated so I just added the changes here

Copy link
Contributor

@ray6080 ray6080 left a comment

Choose a reason for hiding this comment

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

Can you check my comments? I think there are still several critical things need to be addressed to make CopyNode correct and performant. I believe the test failures are related to changes in your PR, and see if my comments can address them.

src/include/processor/operator/copy/read_csv.h Outdated Show resolved Hide resolved
src/include/storage/copier/read_file_state.h Show resolved Hide resolved
src/planner/planner.cpp Outdated Show resolved Hide resolved
src/storage/copier/column_chunk.cpp Outdated Show resolved Hide resolved
src/processor/operator/copy/copy_node.cpp Outdated Show resolved Hide resolved
src/processor/operator/copy/read_file.cpp Outdated Show resolved Hide resolved
src/processor/operator/copy/read_parquet.cpp Outdated Show resolved Hide resolved
src/storage/copier/read_file_state.cpp Show resolved Hide resolved
@aziz-mu aziz-mu requested a review from ray6080 August 7, 2023 03:24
src/storage/copier/rel_copier.cpp Outdated Show resolved Hide resolved
src/storage/copier/column_chunk.cpp Outdated Show resolved Hide resolved
src/storage/copier/read_file_state.cpp Outdated Show resolved Hide resolved
src/storage/copier/read_file_state.cpp Outdated Show resolved Hide resolved
src/processor/operator/copy/read_file.cpp Outdated Show resolved Hide resolved
src/include/common/vector/auxiliary_buffer.h Outdated Show resolved Hide resolved
src/include/common/vector/value_vector.h Outdated Show resolved Hide resolved
src/include/common/vector/value_vector.h Show resolved Hide resolved
src/include/storage/copier/read_file_state.h Show resolved Hide resolved
src/processor/operator/copy/copy_node.cpp Outdated Show resolved Hide resolved
@aziz-mu aziz-mu force-pushed the serial branch 2 times, most recently from 3810d4b to 1f1c2be Compare August 8, 2023 20:10
@ray6080
Copy link
Contributor

ray6080 commented Aug 10, 2023

Can you also check tests under test_files/tck/match and uncomment them? Those tests should also be able to run now after your fix.

@codecov
Copy link

codecov bot commented Aug 11, 2023

Codecov Report

Patch coverage: 69.41% and project coverage change: -0.09% ⚠️

Comparison is base (fc32db0) 89.85% compared to head (ce53e27) 89.77%.
Report is 5 commits behind head on master.

❗ Current head ce53e27 differs from pull request most recent head 678854d. Consider uploading reports for the commit 678854d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1886      +/-   ##
==========================================
- Coverage   89.85%   89.77%   -0.09%     
==========================================
  Files         869      869              
  Lines       31180    31276      +96     
==========================================
+ Hits        28016    28077      +61     
- Misses       3164     3199      +35     
Files Changed Coverage Δ
src/include/common/vector/auxiliary_buffer.h 92.30% <ø> (ø)
src/include/common/vector/value_vector.h 100.00% <ø> (ø)
src/include/storage/store/node_column.h 100.00% <ø> (ø)
src/storage/store/node_column.cpp 79.39% <ø> (+2.70%) ⬆️
src/storage/store/string_node_column.cpp 98.48% <ø> (ø)
src/storage/copier/read_file_state.cpp 64.78% <30.55%> (-35.22%) ⬇️
...lude/planner/logical_plan/copy/logical_copy_from.h 76.92% <60.00%> (+4.19%) ⬆️
src/common/vector/value_vector.cpp 99.45% <100.00%> (-0.02%) ⬇️
...c/include/processor/operator/copy_from/copy_node.h 100.00% <100.00%> (ø)
...rc/include/processor/operator/copy_from/read_csv.h 100.00% <100.00%> (ø)
... and 17 more

... and 7 files with indirect coverage changes

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

@aziz-mu aziz-mu force-pushed the serial branch 3 times, most recently from f6ff049 to d75d451 Compare August 11, 2023 15:17
@aziz-mu aziz-mu merged commit e48ea4f into master Aug 11, 2023
9 of 10 checks passed
@aziz-mu aziz-mu deleted the serial branch August 11, 2023 19:26
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

2 participants