-
Notifications
You must be signed in to change notification settings - Fork 300
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
HPCC-22869 Lightweight join must always ensure sides are ungrouped #13012
Conversation
https://track.hpccsystems.com/browse/HPCC-22869 |
eabaddf
to
8752714
Compare
@ghalliday - pushed to add a regression test. |
@ghalliday @richardkchapman - should be this be 7.2.x? |
Has any 7.2.x user reported it? |
yes, the only report so far was on 7.2.36 |
Bit nervous about adding bugfixes that change the result in a point release. Is there a workaround? |
They succeeded in working around the bug by adding a DEGROUP on the RHS of the join - not sure if guaranteed to remain once optimized though. |
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.
@jakesmith a couple of questions
leftStream.set(leftInputStream); // already ungrouped | ||
else | ||
leftStream.setown(createUngroupStream(leftInputStream)); | ||
leftStream.setown(createUngroupStream(leftInputStream)); |
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.
Is there any way of only doing this if the stream is grouped?
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 guess it is an extra virtual call per row,
and I think I can test input->isGrouped() to make it conditional
|
||
// Lightweight join is only implemented in Thor | ||
//noroxie | ||
//nohthor |
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.
what happens if it is run in roxie/hthor?
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.
it will run and match, but appears not generate a lightweight join..
8752714
to
0b9b1ab
Compare
@ghalliday - have changed it so conditional on isGrouped() Not sure whether the new regression query should bother to run on hthor or roxie ? |
The change looks good to me. I probably would enable the tests for roxie/hthor, even though they don't generate that specific activity kind. |
Signed-off-by: Jake Smith <jake.smith@lexisnexisrisk.com>
0b9b1ab
to
f5e42b6
Compare
@ghalliday - have changed the regression test so also runs on hthor and roxie (and squashed that change in). |
Automated Smoketest: ✅ Unit tests result:
Regression test result:
HPCC Stop: OK
|
Signed-off-by: Jake Smith jake.smith@lexisnexisrisk.com
Type of change:
Checklist:
Smoketest:
Testing: