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.

Sign up for GitHub

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

HPCC-32856 CKeyCursor should link parent nodes as well as current node #19228

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

HPCC-32856 CKeyCursor should link parent nodes as well as current node #19228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 113 additions & 1 deletion system/jhtree/jhtree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1805,6 +1805,11 @@ CKeyCursor::CKeyCursor(const CKeyCursor &from)
{
nodeKey = from.nodeKey;
node.set(from.node);
for (unsigned i = 0; i < maxParentNodes; i++)
{
parents[i].set(from.parents[i]);
parentNodeKeys[i] = from.parentNodeKeys[i];
}
unsigned keySize = key.keySize();
recordBuffer = (char *) malloc(keySize); // MORE - would be nice to know real max - is it stored in metadata?
memcpy(recordBuffer, from.recordBuffer, key.keyedSize()); // Just make keyed portion valid initially
Expand All @@ -1824,6 +1829,7 @@ void CKeyCursor::reset()
{
node.clear();
matched = false;
clearParentNodes();
eof = key.bloomFilterReject(*filter) || !filter->canMatch();
if (!eof)
setLow(0);
Expand All @@ -1840,6 +1846,7 @@ bool CKeyCursor::_next(IContextLogger *ctx)
if (!node)
{
node.setown(key.locateFirstLeafNode(ctx));
clearParentNodes(); // MORE - could do better, but may not be a lot of point?
nodeKey = 0;
return node && node->isKeyAt(nodeKey);
}
Expand All @@ -1857,9 +1864,33 @@ bool CKeyCursor::_next(IContextLogger *ctx)
if (node != NULL)
{
nodeKey = 0;
// Update the nodekey indexes of any parent nodes that are still relevant, and clear any that are not
unsigned depth = key.getBranchDepth();
Copy link
Member

Choose a reason for hiding this comment

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

Would benefit from a comment about what this code is doing....

while (depth)
{
depth--;
if (depth < maxParentNodes && parents[depth])
{
unsigned numParentKeys = parents[depth]->getNumKeys();
unsigned parentNodeKey = parentNodeKeys[depth];
if (parentNodeKey < numParentKeys-1)
{
parentNodeKeys[depth] = parentNodeKey+1;
break;
}
parents[depth].clear();
}
else
{
// We can't tell which parent node info is correct. Clear them all.
clearParentNodes();
break;
}
}
return node->isKeyAt(0);
}
}
clearParentNodes();
return false;
}
else
Expand Down Expand Up @@ -1915,6 +1946,7 @@ bool CKeyCursor::_last(IContextLogger *ctx)
{
fullBufferValid = false;
node.setown(key.locateLastLeafNode(ctx));
clearParentNodes(); // Could in theory do better but unlikely to be any point
if (node)
{
nodeKey = node->getNumKeys()-1;
Expand All @@ -1940,12 +1972,43 @@ bool CKeyCursor::_gtEqual(IContextLogger *ctx)
{
int rc = node->compareValueAt(recordBuffer, ++nodeKey);
if (rc <= 0)
return true;
{
return true;
}
if (nodeKey < numKeys-1)
{
rc = node->compareValueAt(recordBuffer, numKeys-1);
if (rc <= 0)
{
lwm = nodeKey+1;
}
}
}
}
if (!lwm)
{
while (depth)
{
depth--;
if (depth < maxParentNodes)
{
const CJHSearchNode *startnode = parents[depth];
if (startnode)
{
unsigned numParentKeys = startnode->getNumKeys();
unsigned parentNodeKey = parentNodeKeys[depth];
if (parentNodeKey < numParentKeys-1)
{
int rc = startnode->compareValueAt(recordBuffer, numParentKeys-1);
if (rc <= 0)
{
lwm = parentNodeKey;
node.set(startnode);
break;
}
}
parents[depth].clear();
}
}
}
}
Expand Down Expand Up @@ -1978,6 +2041,12 @@ bool CKeyCursor::_gtEqual(IContextLogger *ctx)
if (a<node->getNumKeys())
{
offset_t npos = node->getFPosAt(a);
if (depth < maxParentNodes)
{
parentNodeKeys[depth] = a;
parents[depth].swap(node);
}
lwm = 0;
depth++;
NodeType type = (depth < branchDepth) ? NodeBranch : NodeLeaf;
node.setown(getCursorNode(npos, type, ctx));
Expand Down Expand Up @@ -2019,6 +2088,33 @@ bool CKeyCursor::_ltEqual(IContextLogger *ctx)
}
}
if (!lwm)
{
while (depth)
{
depth--;
if (depth < maxParentNodes)
{
const CJHSearchNode *startnode = parents[depth];
if (startnode)
{
unsigned numParentKeys = startnode->getNumKeys();
unsigned parentNodeKey = parentNodeKeys[depth];
if (parentNodeKey < numParentKeys-1)
{
int rc = startnode->compareValueAt(recordBuffer, numParentKeys-1);
if (rc < 0)
{
lwm = parentNodeKey;
node.set(startnode);
break;
}
}
parents[depth].clear();
}
}
}
}
if (!lwm)
{
node.set(key.rootNode);
depth = 0;
Expand Down Expand Up @@ -2052,6 +2148,12 @@ bool CKeyCursor::_ltEqual(IContextLogger *ctx)
if (a==node->getNumKeys())
a--; // value being looked for is off the end of the index.
offset_t npos = node->getFPosAt(a);
if (depth < maxParentNodes)
{
parentNodeKeys[depth] = a;
parents[depth].swap(node);
}
lwm = 0;
Copy link
Member

Choose a reason for hiding this comment

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

Is this a lurking bug?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, in that previously if lwm was non-zero we were already at leaf level and so would only go around the loop once. So you could argue it was a lurking bug but it's one that would never have been able to trigger previously.

depth++;
NodeType type = (depth < branchDepth) ? NodeBranch : NodeLeaf;
node.setown(getCursorNode(npos, type, ctx));
Expand All @@ -2061,6 +2163,15 @@ bool CKeyCursor::_ltEqual(IContextLogger *ctx)
}
}

void CKeyCursor::clearParentNodes()
{
for (unsigned i = 0; i < maxParentNodes; i++)
{
parents[i].clear();
parentNodeKeys[i] = 0;
}
}

void CKeyCursor::serializeCursorPos(MemoryBuffer &mb)
{
mb.append(eof);
Expand All @@ -2086,6 +2197,7 @@ void CKeyCursor::deserializeCursorPos(MemoryBuffer &mb, IContextLogger *ctx)
{
mb.read(eof);
node.clear();
clearParentNodes(); // we don't serialize parent node info
if (!eof)
{
mb.read(matched);
Expand Down
4 changes: 4 additions & 0 deletions system/jhtree/jhtree.ipp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,13 @@ public:
class jhtree_decl CKeyCursor : public CInterfaceOf<IKeyCursor>, implements INodeLoader
{
protected:
static constexpr unsigned maxParentNodes = 4;
CKeyIndex &key;
const IIndexFilterList *filter;
char *recordBuffer = nullptr;
Owned<const CJHSearchNode> node;
Owned<const CJHSearchNode> parents[maxParentNodes];
unsigned int parentNodeKeys[maxParentNodes] = {0};
unsigned int nodeKey;

mutable bool fullBufferValid = false;
Expand Down Expand Up @@ -267,6 +270,7 @@ protected:
// if _lookup returns true, recordBuffer will contain keyed portion of result
bool _lookup(bool exact, unsigned lastSeg, bool unfiltered, IContextLogger *ctx);

void clearParentNodes();

void reportExcessiveSeeks(unsigned numSeeks, unsigned lastSeg, IContextLogger *ctx);

Expand Down
Loading