FrankChen021 commented on code in PR #20450:
URL: https://github.com/apache/druid/pull/20450#discussion_r4133379749
##########
processing/src/main/java/org/apache/druid/segment/incremental/OnheapIncrementalIndex.java:
##########
@@ -706,6 +713,19 @@ public ColumnValueSelector<?>
makeColumnValueSelector(String columnName)
return existing != null ? existing : columnValueSelector;
}
+ @Override
+ public ColumnValueSelector<ExprEval> getOrCreateExprEvalSelector(final
Expr expression)
+ {
+ ColumnValueSelector<ExprEval> selector =
expressionSelectorMap.get(expression);
+ if (selector == null) {
+ // Do not use computeIfAbsent: building the expression selector can
re-enter makeColumnValueSelector on this
+ // factory through expression bindings.
+ selector = ExpressionSelectors.makeExprEvalSelectorUncached(this,
expression);
+ expressionSelectorMap.put(expression, selector);
Review Comment:
[P2] Avoid sharing cached results for non-deterministic expressions
**Finding:** A longSum metric with expression `now() - __time` now reuses
the first result across distinct rollup keys whenever their input timestamps
are equal. `__time` has LONG capabilities, so ExpressionPlanner marks this
expression SINGLE_INPUT_SCALAR even though its analysis is non-deterministic;
makeExprEvalSelectorUncached therefore builds a
SingleLongInputCachingExpressionColumnValueSelector, whose lastOutput is reused
while __time is unchanged. Previously each new rollup key received a fresh
selector and evaluated now() at its own ingestion time. With this factory-wide
cache, a later row with the same timestamp and a new dimension key instead
records the earlier row's ingestion lag. The same regression occurs with rollup
disabled, where each row previously got a fresh selector.
**Suggestion:** Prevent non-deterministic expressions from using input-value
result caches when sharing selectors, or exclude them from selector reuse; add
coverage for rows arriving at different processing times with equal __time and
distinct keys.
--
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]
