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

Serial fix node group #1863

Closed
wants to merge 35 commits into from
Closed

Serial fix node group #1863

wants to merge 35 commits into from

Conversation

aziz-mu
Copy link
Contributor

@aziz-mu aziz-mu commented Jul 28, 2023

Fixing serial in node-groups branch. Also added a test/dataset for serial copy which required more than one node group

@aziz-mu aziz-mu marked this pull request as ready for review July 28, 2023 15:47
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.

Just took a quick look. I think there are still something missing in this PR before I dive into more detailed review:

  1. You only handled the case for CSV, can you also add the logic to Parquet and NPY?
  2. It seems CombineChunksToBatch is not performing zero-copy internally (found something here, double check their source code? The concatenation can be costful. We should avoid it. Are there any other solutions in your mind? if not, can you try to explore my previous suggestion on using ChunkedArray? For which you need to change ValueVector to be able to keep ChunkedArray instead of Array.
  3. There is no distinction between forcing order preserving or not (has SERIAL or not) in reading from csv files. This might be fine for CSV, as it has to be in a serial mode anyways, but when you replicate changes to Parquet and NPY, we do need to consider the case that we can read concurrently (which is critical) if there is no SERIAL. So better to differentiate the reading logic of the two cases regarding to preserving order or not.

@@ -0,0 +1,2 @@
CALL threads=2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually we don't manually config num of threads for copy. Is there any special reason here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted this test to always fail if this feature wasn't working. Since it's based on a race condition, I found that running it with 2 threads made it a lot more likely to fail than running it with default parameters. Testing with default parameters, this test failed only 5/15 times, and with 2 threads it failed 15/15 times, which is what I wanted.

Not exactly sure why more threads doesn't make it more likely to fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, I know that there is a PARALLELISM command for the tests, but I'm not sure if that affects the actual copy command, which is the command where is issue occurs

src/include/processor/operator/copy/copy_node.h Outdated Show resolved Hide resolved
src/include/processor/operator/copy/copy_node.h Outdated Show resolved Hide resolved
@ray6080 ray6080 force-pushed the node-group branch 3 times, most recently from 00eacad to d03b06a Compare August 2, 2023 08:23
Base automatically changed from node-group to master August 2, 2023 11:18
@aziz-mu aziz-mu closed this Aug 3, 2023
@andyfengHKU andyfengHKU deleted the serial-fix-node-group branch November 6, 2023 08:37
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