kgyrtkirk commented on PR #15552:
URL: https://github.com/apache/druid/pull/15552#issuecomment-1880752148
honestly: I was not thinking that should be counted in when I was making
this change; `ExprEval` seemed like an immutable class to me
I personally think that this will not end in erroreous behaviour
* as it may only happen if `stringValueCached` would get its value later
than `stringValue` (which is not true)
* I believe in this case the only way this could end up causing a
multithreading issue is when new `stringValueCached` value may become visible
while `stringValue` is not
* that's internally is kinda like when the `stringValue` pointer and the
`stringValueCached` primitive boolean is not inside the same cacheLine
* it seemed to me that `cacheLine` size is at least 64 bytes ; but it could
highly depend on the model/etc
```
stringValue = Evals.asString(value);
stringValueCached = true;
```
this reasoning could be wrong and even if it may not happen - it would be
better to fix it; even thru the `ExprEval` class seem to be pretty small.
Did it caused any real trouble (I deeply sorry for the it) - but I would be
very interested in learning more about it if it did!
##### "makeItReallyImmutable" approach
it could achieved with a call to `expr.getCachedStringValue()` ; which is
not nice...probably adding a `makeImmutable()` or something...
##### the `singleThreaded` appraoch
* it seems to me it adds one more extra conditional to be evaluated...but no
big deal I think the branch predictor can do a pretty good job with it..
* wouldn't the above sample still retain the issue in `ExprEval` ? and it
would still need the `makeItReallyImmutable` as well in some form?
I wonder about:
* instead of caching I wonder why not remove this conditional caching layer
-
* if say on a `DoubleExpr` the `asString()` method will be called most of
the time then why have a `DoubleExpr` with caching ? why not a
`ConstantStringExpr` ?
or did you meaned something like:
* pick up `eval` as field `eval`
* have a `singleThreaded` method version on the `DoubleExpr` class as well
something like:
```
// constructor used by singleThreaded
DoubleExpr(ExprEval<?> eval)
{
super(ExpressionType.DOUBLE, Preconditions.checkNotNull(eval.value(),
"value"));
this.eval = eval.singleThreaded();
}
```
I think the `ConstantExpr` should also declare a new `eval` method without
bind arguments and finalize method
note: when I was doing this change I really wanted to do it differently with
a more immutable approach - I really wanted to make it more immutable:
but wasn't able to create these objects early for all cases: as for some odd
reason
[BigIntegerExpr](https://github.com/apache/druid/blob/63bfb3e6c94e218bc20f738e7f4c285e2518b1f9/processing/src/main/java/org/apache/druid/math/expr/ConstantExpr.java#L126-L128)
may *throw* an exception during evalution....but wanted to remain on the safe
side and take my chances - for `BigIntegerExpr` its kinda true that its either
a number which can be represented as a `LONG` or a `STRING` - but never a true
`BigInteger` - or at least that's how I see it now...
I'll keep thinking about it and make some draft PR to address this issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
