Sequential read for StringPropertyList #1327
Conversation
| void DiskOverflowFile::readStringsToVector(TransactionType trxType, ValueVector& valueVector) { | ||
| assert(!valueVector.state->isFlat()); | ||
| OverflowCache overflowCache; |
There was a problem hiding this comment.
I'm slight concerned whether this object creation will cause overhead. Usually the compiler optimize these things so it shouldn't matter.
But can u try to benchmark a bit to see if we become slower if we wrap things as a struct? If we do become slower I would use three primitive types instead.
There was a problem hiding this comment.
Thanks! @rfdavid Looks good to me.
Just two minor comments:
- you wrote in the description
name string has ~ 25kb for each node property, did you mean 25bytes here? I don't think we can store strings whose length are 25KB right now. - can you add one more performance numbers comparison on lists string reading? my understanding is that we should see some performance gains in that case.
We can take another quick look, then we can get this in. Thanks!
| void readStringsToVector( | ||
| transaction::TransactionType trxType, common::ValueVector& valueVector); | ||
| void readStringToVector(transaction::TransactionType trxType, common::ku_string_t& kuStr, | ||
| struct OverflowPageCache { |
There was a problem hiding this comment.
Can you move this struct to private? I guess it doesn't need to be exposed as public for now.
There was a problem hiding this comment.
Hi! Thank you so much for your comments.
- You are right, I actually created a csv with a very long string, but after querying, I realized it was stored only 4096 bytes.
- I'll benchmark it soon. I believe the numbers should be better, considering the cache is implemented for the child type STRING (unless the object creating is causing some overhead, as mentioned by @andyfengHKU)
| unpinOverflowPageCache(overflowPageCache); | ||
| overflowPageCache.frame = bufferManager.pin(*fileHandleToPin, pageIdxToPin); | ||
| overflowPageCache.fileHandle = fileHandleToPin; | ||
| overflowPageCache.pageIdx = pageIdxToPin; |
There was a problem hiding this comment.
I would consider wrapping these three lines as pinOverflowPageCache, but i don't have a strong opinion on this, up to you.
overflowPageCache.frame = bufferManager.pin(*fileHandleToPin, pageIdxToPin);
overflowPageCache.fileHandle = fileHandleToPin;
overflowPageCache.pageIdx = pageIdxToPin;
There was a problem hiding this comment.
I think it makes sense since I already have unpinOverflowPageCache, which definitely increases readability. thanks
rfdavid
force-pushed
the
sequential_read
branch
2 times, most recently
from
cfb0409
to
04564d9
Compare
Also: I have read and agree to the terms under CLA.md
rfdavid
force-pushed
the
sequential_read
branch
from
04564d9
to
b8f50e1
Compare
This PR implements overflow cache in the
DiskOverflowFile::readStringToVectorfunction in the same way that was implemented inscanSequentialStringOverflow., so it can be reused in different parts of the code.Benchmark
CREATE NODE TABLE City(name STRING, population INT64, PRIMARY KEY (name))namestring has ~ 25kb for each node propertyResults of
SCAN_NODE_PROPERTYafter runningPROFILE MATCH (c:City) RETURN c.name;5 times on master branch and on this branch:Related to #756 (note: this only addresses string overflow, not lists)
I have read and agree to the terms under CLA.md