-
Notifications
You must be signed in to change notification settings - Fork 311
HPCC-35431 Avoid using accessors to calculate offsets in compare functions #20691
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
base: candidate-10.0.x
Are you sure you want to change the base?
Conversation
|
Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-35431 Jirabot Action Result: |
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.
Pull request overview
This PR implements a performance optimization to avoid using accessor classes when generating comparison functions in the ECL compiler. Row accessor classes pre-calculate offsets for all fields in a record, but comparison functions often terminate early before accessing all fields, making this pre-calculation wasteful. The change adds a delayOffsetCalculation parameter to bindTableCursor methods that, when set to true, bypasses accessor class creation.
- Adds
delayOffsetCalculationparameter tobindTableCursormethods to control accessor class usage - Applies optimization to
buildReturnOrderfunction which generates comparison methods - Updates inline wrapper functions to maintain backward compatibility with default
falsevalue
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| ecl/hqlcpp/hqlcpp.ipp | Adds avoidAccessor parameter (default false) to bindTableCursor method declarations and updates inline wrapper functions to pass the parameter |
| ecl/hqlcpp/hqlhtcpp.cpp | Implements delayOffsetCalculation parameter in bindTableCursor methods to conditionally disable accessor classes; applies optimization in buildReturnOrder for compare functions |
ecl/hqlcpp/hqlhtcpp.cpp
Outdated
| bindTableCursor(ctx, dataset.queryDataset(), "left", no_left, selSeq); | ||
| bindTableCursor(ctx, dataset.queryDataset(), "right", no_right, selSeq); | ||
| // Avoid using accessor classes with compare functions - because it is highly likely that the | ||
| // function will terminate long before the latter fields are accessed - leaving to significantly |
Copilot
AI
Nov 28, 2025
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.
Typo in comment: "leaving to" should be "leading to".
| // function will terminate long before the latter fields are accessed - leaving to significantly | |
| // function will terminate long before the latter fields are accessed - leading to significantly |
ecl/hqlcpp/hqlcpp.ipp
Outdated
| BoundRow * bindTableCursor(BuildCtx & ctx, IHqlExpression * dataset, const char * bound, bool isLinkCounted, node_operator no_side, IHqlExpression * selSeq, bool avoidAccessor = false); | ||
| BoundRow * bindTableCursor(BuildCtx & ctx, IHqlExpression * dataset, IHqlExpression * bound, node_operator no_side, IHqlExpression * selSeq, bool avoidAccessor = false); |
Copilot
AI
Nov 28, 2025
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.
Parameter name inconsistency: The parameter is named avoidAccessor in the header but delayOffsetCalculation in the implementation (hqlhtcpp.cpp:7408, 7419). These names should match. The implementation name delayOffsetCalculation better describes what the parameter does (delays offset calculation by avoiding accessor classes), so consider updating the header to use delayOffsetCalculation instead of avoidAccessor.
| BoundRow * bindTableCursor(BuildCtx & ctx, IHqlExpression * dataset, const char * bound, bool isLinkCounted, node_operator no_side, IHqlExpression * selSeq, bool avoidAccessor = false); | |
| BoundRow * bindTableCursor(BuildCtx & ctx, IHqlExpression * dataset, IHqlExpression * bound, node_operator no_side, IHqlExpression * selSeq, bool avoidAccessor = false); | |
| BoundRow * bindTableCursor(BuildCtx & ctx, IHqlExpression * dataset, const char * bound, bool isLinkCounted, node_operator no_side, IHqlExpression * selSeq, bool delayOffsetCalculation = false); | |
| BoundRow * bindTableCursor(BuildCtx & ctx, IHqlExpression * dataset, IHqlExpression * bound, node_operator no_side, IHqlExpression * selSeq, bool delayOffsetCalculation = false); |
ecl/hqlcpp/hqlcpp.ipp
Outdated
| { return bindTableCursor(ctx, dataset, bound, false, no_none, NULL, false); } | ||
| inline BoundRow * bindTableCursor(BuildCtx & ctx, IHqlExpression * dataset, IHqlExpression * bound) | ||
| { return bindTableCursor(ctx, dataset, bound, no_none, NULL); } | ||
| { return bindTableCursor(ctx, dataset, bound, no_none, NULL, false); } |
Copilot
AI
Nov 28, 2025
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.
Minor formatting inconsistency: There is an extra space after bound, (two spaces instead of one). Should be bound, no_none to match the formatting of the other inline functions.
| { return bindTableCursor(ctx, dataset, bound, no_none, NULL, false); } | |
| { return bindTableCursor(ctx, dataset, bound, no_none, NULL, false); } |
…tions Signed-off-by: Gavin Halliday <[email protected]>
Type of change:
Checklist:
Smoketest:
Testing: