HPCC-34657 Hybrid index improve how fpos are stored #20650

jpmcmu · 2025-11-14T20:50:11Z

Moved file positions to end of record
Optional removed file positions if they will always be zero

Signed-off-by: James McMullan [email protected]

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

github-actions · 2025-11-15T03:50:56Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-34657

Jirabot Action Result:
Assigning user: [email protected]
Workflow Transition To: Merge Pending
Updated PR

ghalliday

Thanks. Generally looks clean. A few comments/edge cases/optimizations.

ghalliday · 2025-11-18T14:15:30Z

system/jhtree/jhblockcompressed.cpp

-        //fields start at the beginning of the row.
-        memcpy(dst+keyCompareLen, p+keyCompareLen+sizeof(offset_t), keyLen-keyCompareLen);
-        memcpy(dst+keyLen, p, sizeof(offset_t));
+        memcpy(dst+keyCompareLen, p+keyCompareLen, keyLen-keyCompareLen);


optimization: If hasFilePosition this can now to a single memcpy, and the false case can use memset.

Made this change

ghalliday · 2025-11-18T14:18:47Z

system/jhtree/jhblockcompressed.cpp

-    if (0xffff == hdr.numKeys || 0 == compressor.writekey(pos, (const char *)indata, insize))
+    unsigned writeOptions = KeyCompressor::TrailingFilePosition | (context.zeroFilePos ? KeyCompressor::NoFilePosition : 0);
+    int written = compressor.writekey(pos, (const char *)indata, insize, writeOptions);
+    if (0xffff == hdr.numKeys || written == 0)


The test on numKeys is no longer short-circuiting the call to writekey.

ghalliday · 2025-11-18T14:19:36Z

system/jhtree/jhblockcompressed.cpp

    CompressionMethod compressionMethod = *(CompressionMethod*) keys;
    keys += sizeof(CompressionMethod);
+
+    hasFilePosition = *(bool*) keys;


A future PR should generalize this to a byte bitfield so that other information can be added. Not for this though.

Could / should this be added to the key header instead of the node header?

Yes, quite possibly, that can be looked at later.

ghalliday · 2025-11-18T14:21:22Z

system/jhtree/jhblockcompressed.cpp

        hdr.keyBytes += sizeof(context.compressionMethod);
+
+        bool hasFilepos = !context.zeroFilePos;
+        memcpy(keyPtr, &hasFilepos, sizeof(bool));


trivial: More normal style for a single bool would be
*(bool *)keyPtr = hasFilepos;

ghalliday · 2025-11-18T14:22:31Z

system/jhtree/jhblockcompressed.cpp

        {
-            memcpy(dst+keyCompareLen, p+keyCompareLen+sizeof(offset_t), reclen-keyCompareLen);
-            memcpy(dst+reclen, p, sizeof(offset_t));
+            memcpy(dst+keyCompareLen, p+keyCompareLen, reclen-keyCompareLen);


similar optimization to the fixed length above.

ghalliday · 2025-11-18T14:25:10Z

system/jhtree/jhblockcompressed.cpp

    if (!context.compressionHandler)
        throw MakeStringException(0, "Unknown compression method %d", (int)compressionMethod);
+
+    if (helper && (helper->getFlags() & TIWzerofilepos))


Does this also need to take into account whether this is a TLK. TLKs implicitly have no payload, but do have a fileposition.

Add
//version multiPart=true,variant='hybrid'
to stress text and check it still works.

Added to the test, and it passed

ghalliday

@jpmcmu please squash.

jpmcmu · 2025-11-24T16:06:26Z

@ghalliday squashed

ghalliday

Apologies, a couple of final comments.

ghalliday · 2025-11-28T15:26:54Z

system/jhtree/jhblockcompressed.cpp

+        if (hasFilePosition)
+            return keyLen + sizeof(offset_t);
+        else
+            return keyLen;


I think this change is wrong - this is the size as seen by the consumer, which should include the zeroed fileposition

Ah, that makes sense, fixed.

ghalliday · 2025-11-28T15:27:00Z

system/jhtree/jhblockcompressed.cpp

    CompressionMethod compressionMethod = *(CompressionMethod*) keys;
    keys += sizeof(CompressionMethod);
+
+    hasFilePosition = *(bool*) keys;


hasFilePosition is a confusing name - because there is already an option
keyHdr->hasSpecialFileposition()
This should be called zeroFilePosition or something similar.

jpmcmu · 2025-12-01T16:28:48Z

@ghalliday Implemented code review changes, will squash if it looks good

ghalliday

@jpmcmu thanks. Please squash and I will mrge.

- Moved file positions to end of record - Optional removed file positions if they will always be zero Signed-off-by: James McMullan [email protected]

jpmcmu · 2025-12-02T14:14:00Z

@ghalliday Squashed

github-actions · 2025-12-03T09:09:05Z

Jirabot Action Result:
Fix versions already added.
Workflow Transition: 'Resolve issue'

Copilot AI review requested due to automatic review settings November 14, 2025 20:50

Copilot AI reviewed Nov 14, 2025

View reviewed changes

jpmcmu requested a review from Copilot November 14, 2025 21:53

Copilot AI reviewed Nov 14, 2025

View reviewed changes

Copilot started reviewing on behalf of jpmcmu November 15, 2025 06:10 View session

Copilot finished reviewing on behalf of jpmcmu November 15, 2025 06:13

Copilot started reviewing on behalf of jpmcmu November 15, 2025 06:13 View session

Copilot finished reviewing on behalf of jpmcmu November 15, 2025 06:15

jpmcmu force-pushed the HPCC-34657 branch from f126e8b to 72ee608 Compare November 17, 2025 18:37

jpmcmu requested a review from ghalliday November 17, 2025 18:38

ghalliday requested changes Nov 18, 2025

View reviewed changes

jpmcmu force-pushed the HPCC-34657 branch from 106091a to 5fafaf7 Compare November 20, 2025 23:27

jpmcmu requested a review from ghalliday November 20, 2025 23:31

ghalliday approved these changes Nov 24, 2025

View reviewed changes

jpmcmu force-pushed the HPCC-34657 branch from 5fafaf7 to 3606485 Compare November 24, 2025 16:05

ghalliday requested changes Nov 28, 2025

View reviewed changes

jpmcmu requested a review from ghalliday December 1, 2025 16:28

ghalliday approved these changes Dec 2, 2025

View reviewed changes

HPCC-34657 Hybrid index improve how fpos are stored

ca1585b

- Moved file positions to end of record - Optional removed file positions if they will always be zero Signed-off-by: James McMullan [email protected]

jpmcmu force-pushed the HPCC-34657 branch from 8efb1e1 to ca1585b Compare December 2, 2025 14:13

ghalliday merged commit faf91c3 into hpcc-systems:candidate-9.14.x Dec 3, 2025
50 of 51 checks passed

HPCC-34657 Hybrid index improve how fpos are stored #20650

HPCC-34657 Hybrid index improve how fpos are stored #20650

Conversation

jpmcmu commented Nov 14, 2025

Type of change:

Checklist:

Smoketest:

Testing:

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 15, 2025

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

jpmcmu commented Nov 24, 2025

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jpmcmu commented Dec 1, 2025

Uh oh!

ghalliday left a comment

Choose a reason for hiding this comment

Uh oh!

jpmcmu commented Dec 2, 2025

Uh oh!

Uh oh!

github-actions bot commented Dec 3, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants