Skip to content

Commit 5aca3d7

Browse files
costincostin
committed
Replace TimeValue with Duration
To simplify date arithmetic and since the time unit is not necessary, TimeValue has been replaced with java.time.Duration both in the parser and throughout the AST
1 parent d0bfd4c commit 5aca3d7

File tree

9 files changed

+84
-136
lines changed

9 files changed

+84
-136
lines changed

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/expression/promql/subquery/Subquery.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@
88
package org.elasticsearch.xpack.esql.expression.promql.subquery;
99

1010
import org.elasticsearch.common.io.stream.StreamOutput;
11-
import org.elasticsearch.core.TimeValue;
1211
import org.elasticsearch.xpack.esql.EsqlIllegalArgumentException;
1312
import org.elasticsearch.xpack.esql.core.tree.NodeInfo;
1413
import org.elasticsearch.xpack.esql.core.tree.Source;
@@ -17,25 +16,26 @@
1716
import org.elasticsearch.xpack.esql.plan.logical.promql.selector.Evaluation;
1817

1918
import java.io.IOException;
19+
import java.time.Duration;
2020
import java.util.Objects;
2121

2222
public class Subquery extends UnaryPlan {
23-
private final TimeValue range;
24-
private final TimeValue resolution;
23+
private final Duration range;
24+
private final Duration resolution;
2525
private final Evaluation evaluation;
2626

27-
public Subquery(Source source, LogicalPlan child, TimeValue range, TimeValue resolution, Evaluation evaluation) {
27+
public Subquery(Source source, LogicalPlan child, Duration range, Duration resolution, Evaluation evaluation) {
2828
super(source, child);
2929
this.range = range;
3030
this.resolution = resolution;
3131
this.evaluation = evaluation;
3232
}
3333

34-
public TimeValue range() {
34+
public Duration range() {
3535
return range;
3636
}
3737

38-
public TimeValue resolution() {
38+
public Duration resolution() {
3939
return resolution;
4040
}
4141

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/promql/PromqlArithmeticUtils.java‎

Lines changed: 2 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77

88
package org.elasticsearch.xpack.esql.parser.promql;
99

10-
import org.elasticsearch.core.TimeValue;
1110
import org.elasticsearch.xpack.esql.core.expression.predicate.operator.arithmetic.Arithmetics;
1211
import org.elasticsearch.xpack.esql.core.tree.Source;
1312
import org.elasticsearch.xpack.esql.parser.ParsingException;
@@ -27,16 +26,12 @@ public class PromqlArithmeticUtils {
2726
* Evaluate arithmetic operation between two scalar values at parse time.
2827
*
2928
* @param source Source location for error messages
30-
* @param left Left operand (Number, Duration, or TimeValue)
31-
* @param right Right operand (Number, Duration, or TimeValue)
29+
* @param left Left operand (Number or Duration)
30+
* @param right Right operand (Number or Duration)
3231
* @param operation The arithmetic operation
3332
* @return Result value (Number or Duration)
3433
*/
3534
public static Object evaluate(Source source, Object left, Object right, ArithmeticOperation operation) {
36-
// Normalize TimeValue to Duration for consistent handling
37-
left = normalizeToStandardType(left);
38-
right = normalizeToStandardType(right);
39-
4035
// Dispatch to appropriate handler based on operand types
4136
if (left instanceof Duration leftDuration) {
4237
if (right instanceof Duration rightDuration) {
@@ -60,16 +55,6 @@ public static Object evaluate(Source source, Object left, Object right, Arithmet
6055
);
6156
}
6257

63-
/**
64-
* Normalize TimeValue to Duration for consistent internal handling.
65-
*/
66-
private static Object normalizeToStandardType(Object value) {
67-
if (value instanceof TimeValue tv) {
68-
return Duration.ofSeconds(tv.getSeconds());
69-
}
70-
return value;
71-
}
72-
7358
/**
7459
* Duration op Duration (only ADD and SUB supported).
7560
*/
@@ -181,18 +166,4 @@ private static void validatePositiveDuration(Source source, Duration duration) {
181166
throw new ParsingException(source, "Duration must be positive, got [{}]", duration);
182167
}
183168
}
184-
185-
/**
186-
* Convert Duration to TimeValue for parser output.
187-
*/
188-
public static TimeValue durationToTimeValue(Duration duration) {
189-
return TimeValue.timeValueSeconds(duration.getSeconds());
190-
}
191-
192-
/**
193-
* Convert TimeValue to Duration for internal operations.
194-
*/
195-
public static Duration timeValueToDuration(TimeValue timeValue) {
196-
return Duration.ofSeconds(timeValue.getSeconds());
197-
}
198169
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/promql/PromqlExpressionBuilder.java‎

Lines changed: 16 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@
1010
import org.antlr.v4.runtime.ParserRuleContext;
1111
import org.antlr.v4.runtime.tree.ParseTree;
1212
import org.elasticsearch.common.time.DateUtils;
13-
import org.elasticsearch.core.TimeValue;
1413
import org.elasticsearch.xpack.esql.core.InvalidArgumentException;
1514
import org.elasticsearch.xpack.esql.core.QlIllegalArgumentException;
1615
import org.elasticsearch.xpack.esql.core.expression.Expression;
@@ -20,7 +19,6 @@
2019
import org.elasticsearch.xpack.esql.core.type.DataType;
2120
import org.elasticsearch.xpack.esql.core.util.StringUtils;
2221
import org.elasticsearch.xpack.esql.parser.ParsingException;
23-
import org.elasticsearch.xpack.esql.parser.PromqlBaseParser;
2422
import org.elasticsearch.xpack.esql.parser.PromqlBaseParser.HexLiteralContext;
2523
import org.elasticsearch.xpack.esql.parser.PromqlBaseParser.IntegerLiteralContext;
2624
import org.elasticsearch.xpack.esql.parser.PromqlBaseParser.LabelListContext;
@@ -34,7 +32,6 @@
3432
import java.time.Instant;
3533
import java.util.List;
3634
import java.util.Locale;
37-
import java.util.concurrent.TimeUnit;
3835

3936
import static java.util.Collections.emptyList;
4037
import static org.elasticsearch.xpack.esql.parser.ParserUtils.typedParsing;
@@ -99,7 +96,7 @@ public Evaluation visitEvaluation(EvaluationContext ctx) {
9996
return Evaluation.NONE;
10097
}
10198

102-
TimeValue offset = null;
99+
Duration offset = null;
103100
boolean negativeOffset = false;
104101
Instant at = null;
105102

@@ -111,16 +108,10 @@ public Evaluation visitEvaluation(EvaluationContext ctx) {
111108
} else if (atCtx.AT_END() != null) {
112109
at = end;
113110
} else {
114-
TimeValue timeValue = visitTimeValue(atCtx.timeValue());
111+
Duration timeValue = visitTimeValue(atCtx.timeValue());
115112
// the value can have a floating point
116-
long seconds = timeValue.seconds();
117-
double secondFrac = timeValue.secondsFrac(); // convert to nanoseconds
118-
long nanos = 0;
119-
120-
if (secondFrac >= Long.MAX_VALUE / 1_000_000 || secondFrac <= Long.MIN_VALUE / 1_000_000) {
121-
throw new ParsingException(source, "Decimal value [{}] is too large/precise", secondFrac);
122-
}
123-
nanos = (long) (secondFrac * 1_000_000_000);
113+
long seconds = timeValue.getSeconds();
114+
int nanos = timeValue.getNano();
124115

125116
if (atCtx.MINUS() != null) {
126117
if (seconds == Long.MIN_VALUE) {
@@ -141,17 +132,17 @@ public Evaluation visitEvaluation(EvaluationContext ctx) {
141132
}
142133

143134
@Override
144-
public TimeValue visitDuration(DurationContext ctx) {
135+
public Duration visitDuration(DurationContext ctx) {
145136
if (ctx == null) {
146137
return null;
147138
}
148139
Object o = visit(ctx.expression());
149140

150141
return switch (o) {
151-
case TimeValue tv -> tv;
142+
case Duration duration -> duration;
152143
case Literal l -> throw new ParsingException(
153144
source(ctx),
154-
"Expected literals to be already converted to timevalue, got [{}]",
145+
"Expected literals to be already converted to duration, got [{}]",
155146
l.value()
156147
);
157148
case LogicalPlan plan -> {
@@ -162,7 +153,7 @@ public TimeValue visitDuration(DurationContext ctx) {
162153

163154
// Handle Duration
164155
if (value instanceof Duration duration) {
165-
yield PromqlArithmeticUtils.durationToTimeValue(duration);
156+
yield duration;
166157
}
167158

168159
// Handle numeric scalars interpreted as seconds
@@ -171,7 +162,7 @@ public TimeValue visitDuration(DurationContext ctx) {
171162
if (seconds <= 0) {
172163
throw new ParsingException(source(ctx), "Duration must be positive, got [{}]s", seconds);
173164
}
174-
yield TimeValue.timeValueSeconds(seconds);
165+
yield Duration.ofSeconds(seconds);
175166
}
176167

177168
throw new ParsingException(
@@ -191,11 +182,8 @@ public TimeValue visitDuration(DurationContext ctx) {
191182
// Fallback for Expression (shouldn't happen with new logic)
192183
if (e.foldable()) {
193184
Object folded = e.fold(FoldContext.small());
194-
if (folded instanceof TimeValue timeValue) {
195-
yield timeValue;
196-
}
197185
if (folded instanceof Duration duration) {
198-
yield PromqlArithmeticUtils.durationToTimeValue(duration);
186+
yield duration;
199187
}
200188
}
201189
// otherwise bail out
@@ -206,7 +194,7 @@ public TimeValue visitDuration(DurationContext ctx) {
206194
}
207195

208196
@Override
209-
public TimeValue visitTimeValue(TimeValueContext ctx) {
197+
public Duration visitTimeValue(TimeValueContext ctx) {
210198
if (ctx.number() != null) {
211199
var literal = typedParsing(this, ctx.number(), Literal.class);
212200
Number number = (Number) literal.value();
@@ -224,7 +212,7 @@ public TimeValue visitTimeValue(TimeValueContext ctx) {
224212
}
225213
}
226214

227-
return new TimeValue(number.longValue(), TimeUnit.SECONDS);
215+
return Duration.ofSeconds(number.longValue());
228216
}
229217
String timeString = null;
230218
Source source;
@@ -317,11 +305,11 @@ public Literal visitString(StringContext ctx) {
317305
return new Literal(source, string(ctx.STRING()), DataType.KEYWORD);
318306
}
319307

320-
private static TimeValue parseTimeValue(Source source, String text) {
321-
TimeValue timeValue = PromqlParserUtils.parseTimeValue(source, text);
322-
if (timeValue.duration() == 0) {
308+
private static Duration parseTimeValue(Source source, String text) {
309+
Duration duration = PromqlParserUtils.parseDuration(source, text);
310+
if (duration.isZero()) {
323311
throw new ParsingException(source, "Invalid time duration [{}], zero value specified", text);
324312
}
325-
return timeValue;
313+
return duration;
326314
}
327315
}

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/promql/PromqlLogicalPlanBuilder.java‎

Lines changed: 10 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99

1010
import org.antlr.v4.runtime.ParserRuleContext;
1111
import org.antlr.v4.runtime.tree.ParseTree;
12-
import org.elasticsearch.core.TimeValue;
1312
import org.elasticsearch.xpack.esql.core.expression.Expression;
1413
import org.elasticsearch.xpack.esql.core.expression.FoldContext;
1514
import org.elasticsearch.xpack.esql.core.expression.Literal;
@@ -108,11 +107,7 @@ private LogicalPlan wrapLiteral(ParseTree ctx) {
108107
return switch (result) {
109108
case LogicalPlan plan -> plan;
110109
case Literal literal -> new LiteralSelector(source, literal);
111-
case TimeValue tv -> {
112-
// Convert TimeValue to Duration for TIME_DURATION datatype
113-
Duration duration = PromqlArithmeticUtils.timeValueToDuration(tv);
114-
yield new LiteralSelector(source, new Literal(source, duration, DataType.TIME_DURATION));
115-
}
110+
case Duration duration -> new LiteralSelector(source, new Literal(source, duration, DataType.TIME_DURATION));
116111
case Expression expr -> throw new ParsingException(
117112
source,
118113
"Expected a plan or literal, got expression [{}]",
@@ -196,9 +191,8 @@ public LogicalPlan visitSelector(PromqlBaseParser.SelectorContext ctx) {
196191
var durationCtx = ctx.duration();
197192
Expression rangeEx = null;
198193
if (durationCtx != null) {
199-
TimeValue range = visitDuration(durationCtx);
200-
// TODO: TimeValue might not be needed after all
201-
rangeEx = new Literal(source(ctx.duration()), Duration.ofSeconds(range.getSeconds()), DataType.TIME_DURATION);
194+
Duration range = visitDuration(durationCtx);
195+
rangeEx = new Literal(source(ctx.duration()), range, DataType.TIME_DURATION);
202196
}
203197

204198
final LabelMatchers matchers = new LabelMatchers(labels);
@@ -486,16 +480,16 @@ public Subquery visitSubquery(PromqlBaseParser.SubqueryContext ctx) {
486480
}
487481

488482
Evaluation evaluation = visitEvaluation(ctx.evaluation());
489-
TimeValue rangeEx = visitDuration(ctx.range);
490-
TimeValue resolution = visitSubqueryResolution(ctx.subqueryResolution());
483+
Duration rangeEx = visitDuration(ctx.range);
484+
Duration resolution = visitSubqueryResolution(ctx.subqueryResolution());
491485

492486
return new Subquery(source(ctx), plan, rangeEx, resolution, evaluation);
493487
}
494488

495489
/**
496490
* Parse subquery resolution, reusing the same expression folding logic used for duration arithmetic.
497491
*/
498-
public TimeValue visitSubqueryResolution(PromqlBaseParser.SubqueryResolutionContext ctx) {
492+
public Duration visitSubqueryResolution(PromqlBaseParser.SubqueryResolutionContext ctx) {
499493
if (ctx == null) {
500494
return null;
501495
}
@@ -514,25 +508,22 @@ public TimeValue visitSubqueryResolution(PromqlBaseParser.SubqueryResolutionCont
514508
// Parse the base time value (e.g., ":5m" -> "5m")
515509
String timeString = timeCtx.getText().substring(1).trim();
516510
Source timeSource = source(timeCtx);
517-
TimeValue baseValue = PromqlParserUtils.parseTimeValue(timeSource, timeString);
511+
Duration baseValue = PromqlParserUtils.parseDuration(timeSource, timeString);
518512

519513
if (ctx.op == null || ctx.expression() == null) {
520514
return baseValue;
521515
}
522516

523-
// Convert TimeValue to Duration for arithmetic
524-
Duration leftDuration = PromqlArithmeticUtils.timeValueToDuration(baseValue);
525-
526517
// Evaluate right expression
527518
Object rightValue = unwrapLiteral(ctx.expression()).value();
528519

529520
// Perform arithmetic using utility
530521
ArithmeticOperation operation = ArithmeticOperation.fromTokenType(source(ctx.op), ctx.op.getType());
531-
Object result = PromqlArithmeticUtils.evaluate(source(ctx), leftDuration, rightValue, operation);
522+
Object result = PromqlArithmeticUtils.evaluate(source(ctx), baseValue, rightValue, operation);
532523

533-
// Convert result back to TimeValue
524+
// Result should be Duration
534525
if (result instanceof Duration duration) {
535-
return PromqlArithmeticUtils.durationToTimeValue(duration);
526+
return duration;
536527
}
537528

538529
throw new ParsingException(source(ctx), "Expected duration result, got [{}]",

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/parser/promql/PromqlParserUtils.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,12 +7,12 @@
77

88
package org.elasticsearch.xpack.esql.parser.promql;
99

10-
import org.elasticsearch.core.TimeValue;
1110
import org.elasticsearch.core.Tuple;
1211
import org.elasticsearch.xpack.esql.core.tree.Location;
1312
import org.elasticsearch.xpack.esql.core.tree.Source;
1413
import org.elasticsearch.xpack.esql.parser.ParsingException;
1514

15+
import java.time.Duration;
1616
import java.util.LinkedHashMap;
1717
import java.util.Map;
1818

@@ -39,7 +39,7 @@ public class PromqlParserUtils {
3939

4040
private PromqlParserUtils() {}
4141

42-
public static TimeValue parseTimeValue(Source source, String string) {
42+
public static Duration parseDuration(Source source, String string) {
4343
char[] chars = string.toCharArray();
4444

4545
long millis = 0;
@@ -109,7 +109,7 @@ public static TimeValue parseTimeValue(Source source, String string) {
109109
millis += number * msMultiplier;
110110
}
111111

112-
return new TimeValue(millis);
112+
return Duration.ofMillis(millis);
113113
}
114114

115115
static String unquote(Source source) {

‎x-pack/plugin/esql/src/main/java/org/elasticsearch/xpack/esql/plan/logical/promql/PromqlCommand.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,7 @@ public void postAnalysisVerification(Failures failures) {
140140
if (s.labelMatchers().nameLabel().matcher().isRegex()) {
141141
failures.add(fail(s, "regex label selectors on __name__ are not supported at this time [{}]", s.sourceText()));
142142
}
143-
if (s.evaluation() != null && s.evaluation().offset() != null && s.evaluation().offset() != TimeValue.ZERO) {
143+
if (s.evaluation() != null && s.evaluation().offset() != null && s.evaluation().offset().isZero() == false) {
144144
failures.add(fail(s, "offset modifiers are not supported at this time [{}]", s.sourceText()));
145145
}
146146
});

0 commit comments

Comments
 (0)