-
Notifications
You must be signed in to change notification settings - Fork 86
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
Serial fix node group #1863
Conversation
…less storage and wal utils
There was a problem hiding this 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:
- You only handled the case for CSV, can you also add the logic to Parquet and NPY?
- 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 usingChunkedArray
? For which you need to change ValueVector to be able to keepChunkedArray
instead ofArray
. - 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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/planner/logical_plan/logical_operator/logical_copy.h
Outdated
Show resolved
Hide resolved
2e12233
to
955842f
Compare
00eacad
to
d03b06a
Compare
Fixing serial in node-groups branch. Also added a test/dataset for serial copy which required more than one node group