HPCC-15749 Add new EPlikelihood property #8804
Conversation
|
https://track.hpccsystems.com/browse/HPCC-15749 |
| @@ -789,7 +789,8 @@ enum ExprPropKind | |||
| EPunadorned, | |||
| EPlocationIndependent, | |||
| EPmeta, | |||
| EPmax | |||
| EPmax, | |||
There was a problem hiding this comment.
EPmax needs to be the last one - we often use an enumeration like this at the end of a list to indicate the number of elements.
|
@ghalliday Please can you review. |
| return value->getRealValue(); | ||
| } | ||
|
|
||
| double likelihoodValue = -1.0; |
There was a problem hiding this comment.
Efficiency. I would move the declaration of likelihoodExpr above the case statement, and only set it from the value if it is non null
|
@shamser that is looking much closer. A lot of the comments are related to avoiding creating IHqlExpressions to represent doubles - since that is a fairly expensive operation. |
| if (match) | ||
| { | ||
| IHqlExpression * likelihoodExpr = static_cast<IHqlExpression *>(match); | ||
| IValue * value = likelihoodExpr->queryChild(0)->queryValue(); |
There was a problem hiding this comment.
I'm not sure the queryChild(0) is correct. Try calling the function multiple times and check the same results is returned
shamser
force-pushed
the
issue15747c
branch
2 times, most recently
from
21b9c41
to
dfdc48a
Compare
|
@ghalliday Please can you check it over again. |
| @@ -12159,6 +12174,27 @@ extern IHqlExpression *createConstant(IValue * constant) | |||
| return CHqlConstant::makeConstant(constant); | |||
| } | |||
|
|
|||
| extern IHqlExpression *createConstantLikelihoodUnknown() | |||
There was a problem hiding this comment.
These should either be query... or return the expression linked.
|
@ghalliday Please can you review |
| likelihoodExpr.set(queryConstantLikelihoodFalse()); | ||
| break; | ||
| default: | ||
| likelihoodExpr.set(queryConstantLikelihoodUnknown()); |
There was a problem hiding this comment.
minor: Should have a break; for consistency.
|
|
||
| double queryLikelihood(IHqlExpression * expr) | ||
| { | ||
| LinkedHqlExpr likelihoodExpr = queryLikelihoodExpr(expr); |
There was a problem hiding this comment.
minor: more efficient to assign to an IHqlExpression *
Signed-off-by: Shamser Ahmed <shamser.ahmed@lexisnexis.co.uk>
|
Automated Smoketest |
|
I might revisit the format of the graph attribute, but fine as it stands. |
Signed-off-by: Shamser Ahmed shamser.ahmed@lexisnexis.co.uk