-
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-17651 Add --fastsyntax to delay expansion of functions #10013
HPCC-17651 Add --fastsyntax to delay expansion of functions #10013
Conversation
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
https://track.hpccsystems.com/browse/HPCC-17651 |
@richardkchapman this is a first PR which adds the option, but when the option is enabled it still fails on a large number of examples. Should I also add it to the help text or include that in 16711? Assigning for shamser to review for the moment. |
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.
@ghalliday Code reviewed: not a sure about a few things. please can you clarify.
ecl/hql/hqlgram2.cpp
Outdated
@@ -6327,6 +6337,7 @@ IHqlExpression *HqlGram::bindParameters(const attribute & errpos, IHqlExpression | |||
if (requireLateBind(function, actuals)) | |||
{ | |||
IHqlExpression * ret = NULL; | |||
const bool expandCallsWhenBound = true; |
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.
@ghalliday Is this local expandCallsWhenBound supposed to be set here? If so, the next IF section (lines 6330-6340) wil not be executed.
ecl/hql/hqlgram2.cpp
Outdated
@@ -6598,6 +6609,7 @@ IHqlExpression * HqlGram::checkParameter(const attribute * errpos, IHqlExpressio | |||
formalType = formalType->queryChildType(); | |||
if (!formalType->assignableFrom(actualType->queryPromotedType())) | |||
{ | |||
|
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.
@ghalliday Formatting...extra blank line.
@@ -1970,7 +1971,7 @@ IHqlExpression * NewHqlTransformer::transformCall(IHqlExpression * expr) | |||
bool same = transformChildren(body, args); | |||
if (same && (oldFuncdef == newFuncdef)) | |||
return LINK(expr); | |||
OwnedHqlExpr newCall = createBoundFunction(NULL, newFuncdef, args, NULL, DEFAULT_EXPAND_CALL); | |||
OwnedHqlExpr newCall = createBoundFunction(NULL, newFuncdef, args, NULL, false); |
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.
@ghalliday This isn't using the delay expansion flag... should it?
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.
think it is correct. If no_call is being transformed it should remain as a no_call.
@@ -2911,7 +2911,7 @@ IHqlExpression * ImplicitProjectTransformer::createTransformed(IHqlExpression * | |||
OwnedHqlExpr newBody = replaceChild(body, 0, newBodyCode); | |||
OwnedHqlExpr newFuncdef = replaceChild(funcdef, 0, newBody); | |||
unwindChildren(args, expr, 0); | |||
transformed.setown(createBoundFunction(NULL, newFuncdef, args, NULL, DEFAULT_EXPAND_CALL)); | |||
transformed.setown(createBoundFunction(NULL, newFuncdef, args, NULL, false)); |
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.
@ghalliday This isn't using the delay expansion flag... should it?
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.
no, I think it is correct. If no_call has made it through to here it should stay as a no_call.
Signed-off-by: Gavin Halliday <gavin.halliday@lexisnexis.com>
@shamser see commit and comments |
Automated Smoketest: ✅
Install hpccsystems-platform-community_6.3.0-trunk0.el7.x86_64.rpm Unit tests result:
Regression test result:
HPCC Stop: OK |
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.
@richardkchapman Code reviewed ok
Signed-off-by: Gavin Halliday gavin.halliday@lexisnexis.com
Type of change:
Checklist:
Testing:
Run compile regression suites (without specifiying --fastsyntax). Only difference is some warnings about input dataset not being sorted are suppressed (a good thing). Many examples also work with --fastsyntax option enabled, but a work in progress (see issue16711 for details).