Skip to content

Commit

Permalink
Add aggregator fixes and some useless unit tests.
Browse files Browse the repository at this point in the history
Signed-off-by: Yury-Fridlyand <yuryf@bitquilltech.com>
  • Loading branch information
Yury-Fridlyand committed Oct 26, 2022
1 parent bfad32c commit 3eba57e
Show file tree
Hide file tree
Showing 6 changed files with 87 additions and 11 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

package org.opensearch.sql.opensearch.response.agg;

import static org.opensearch.sql.opensearch.response.agg.Utils.handleNanValue;
import static org.opensearch.sql.opensearch.response.agg.Utils.handleNanInfValue;

import java.util.Collections;
import java.util.Map;
Expand All @@ -34,6 +34,6 @@ public class SingleValueParser implements MetricParser {
public Map<String, Object> parse(Aggregation agg) {
return Collections.singletonMap(
agg.getName(),
handleNanValue(((NumericMetricsAggregation.SingleValue) agg).value()));
handleNanInfValue(((NumericMetricsAggregation.SingleValue) agg).value()));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@

package org.opensearch.sql.opensearch.response.agg;

import static org.opensearch.sql.opensearch.response.agg.Utils.handleNanValue;
import static org.opensearch.sql.opensearch.response.agg.Utils.handleNanInfValue;

import java.util.Collections;
import java.util.Map;
Expand All @@ -36,6 +36,6 @@ public class StatsParser implements MetricParser {
@Override
public Map<String, Object> parse(Aggregation agg) {
return Collections.singletonMap(
agg.getName(), handleNanValue(valueExtractor.apply((ExtendedStats) agg)));
agg.getName(), handleNanInfValue(valueExtractor.apply((ExtendedStats) agg)));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@
@UtilityClass
public class Utils {
/**
* Utils to handle Nan Value.
* @return null if is Nan.
* Utils to handle Nan/Infinite Value.
* @return null if is Nan or is +-Infinity.
*/
public static Object handleNanValue(double value) {
return Double.isNaN(value) ? null : value;
public static Object handleNanInfValue(double value) {
return Double.isNaN(value) || Double.isInfinite(value) ? null : value;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,17 @@

package org.opensearch.sql.opensearch.storage.script.aggregation;

import static java.time.temporal.ChronoUnit.MILLIS;

import java.time.LocalTime;
import java.util.Map;
import lombok.EqualsAndHashCode;
import org.apache.lucene.index.LeafReaderContext;
import org.opensearch.script.AggregationScript;
import org.opensearch.search.lookup.SearchLookup;
import org.opensearch.sql.data.model.ExprNullValue;
import org.opensearch.sql.data.model.ExprValue;
import org.opensearch.sql.data.type.ExprCoreType;
import org.opensearch.sql.expression.Expression;
import org.opensearch.sql.expression.env.Environment;
import org.opensearch.sql.opensearch.storage.script.core.ExpressionScript;
Expand Down Expand Up @@ -42,7 +46,17 @@ public ExpressionAggregationScript(

@Override
public Object execute() {
return expressionScript.execute(this::getDoc, this::evaluateExpression).value();
var expr = expressionScript.execute(this::getDoc, this::evaluateExpression);
switch ((ExprCoreType)expr.type()) {
case DATE:
case DATETIME:
case TIMESTAMP:
return expr.timestampValue().toEpochMilli();
case TIME:
return MILLIS.between(LocalTime.MIN, expr.timeValue());
default:
return expr.value();
}
}

private ExprValue evaluateExpression(Expression expression, Environment<Expression,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
import static org.junit.jupiter.api.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.opensearch.sql.opensearch.response.AggregationResponseUtils.fromJson;
import static org.opensearch.sql.opensearch.response.agg.Utils.handleNanValue;
import static org.opensearch.sql.opensearch.response.agg.Utils.handleNanInfValue;

import com.google.common.collect.ImmutableMap;
import java.util.List;
Expand Down Expand Up @@ -161,7 +161,9 @@ void unsupported_aggregation_should_fail() {

@Test
void nan_value_should_return_null() {
assertNull(handleNanValue(Double.NaN));
assertNull(handleNanInfValue(Double.NaN));
assertNull(handleNanInfValue(Double.NEGATIVE_INFINITY));
assertNull(handleNanInfValue(Double.POSITIVE_INFINITY));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,31 @@

package org.opensearch.sql.opensearch.storage.script.aggregation;

import static java.time.temporal.ChronoUnit.MILLIS;
import static java.util.Collections.emptyMap;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;
import static org.opensearch.sql.data.type.ExprCoreType.DATE;
import static org.opensearch.sql.data.type.ExprCoreType.DATETIME;
import static org.opensearch.sql.data.type.ExprCoreType.INTEGER;
import static org.opensearch.sql.data.type.ExprCoreType.STRING;
import static org.opensearch.sql.data.type.ExprCoreType.TIME;
import static org.opensearch.sql.data.type.ExprCoreType.TIMESTAMP;
import static org.opensearch.sql.expression.DSL.literal;
import static org.opensearch.sql.expression.DSL.ref;
import static org.opensearch.sql.opensearch.data.type.OpenSearchDataType.OPENSEARCH_TEXT_KEYWORD;

import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;

import java.time.LocalDate;
import java.time.LocalDateTime;
import java.time.LocalTime;
import java.util.Arrays;
import java.util.List;
import java.util.Map;
import lombok.RequiredArgsConstructor;
import org.apache.lucene.index.LeafReaderContext;
Expand All @@ -32,8 +44,17 @@
import org.opensearch.search.lookup.LeafDocLookup;
import org.opensearch.search.lookup.LeafSearchLookup;
import org.opensearch.search.lookup.SearchLookup;
import org.opensearch.sql.data.model.ExprDateValue;
import org.opensearch.sql.data.model.ExprDatetimeValue;
import org.opensearch.sql.data.model.ExprTimeValue;
import org.opensearch.sql.data.model.ExprTimestampValue;
import org.opensearch.sql.data.model.ExprValue;
import org.opensearch.sql.data.model.ExprValueUtils;
import org.opensearch.sql.expression.DSL;
import org.opensearch.sql.expression.Expression;
import org.opensearch.sql.expression.aggregation.AggregationState;
import org.opensearch.sql.expression.aggregation.Aggregator;
import org.opensearch.sql.expression.aggregation.MinAggregator;
import org.opensearch.sql.expression.config.ExpressionConfig;

@DisplayNameGeneration(DisplayNameGenerator.ReplaceUnderscores.class)
Expand Down Expand Up @@ -103,6 +124,45 @@ void can_execute_parse_expression() {
.shouldMatch("30");
}

@Test
void can_execute_expression_interpret_dates_for_aggregation() {
assertThat()
.docValues("date", "1961-04-12")
.evaluate(
dsl.date(ref("date", STRING)))
.shouldMatch(new ExprDateValue(LocalDate.of(1961, 4, 12))
.timestampValue().toEpochMilli());
}

@Test
void can_execute_expression_interpret_datetimes_for_aggregation() {
assertThat()
.docValues("datetime", "1984-03-17 22:16:42")
.evaluate(
dsl.datetime(ref("datetime", STRING)))
.shouldMatch(new ExprDatetimeValue("1984-03-17 22:16:42")
.timestampValue().toEpochMilli());
}

@Test
void can_execute_expression_interpret_times_for_aggregation() {
assertThat()
.docValues("time", "22:13:42")
.evaluate(
dsl.time(ref("time", STRING)))
.shouldMatch(MILLIS.between(LocalTime.MIN, LocalTime.of(22, 13, 42)));
}

@Test
void can_execute_expression_interpret_timestamps_for_aggregation() {
assertThat()
.docValues("timestamp", "1984-03-17 22:16:42")
.evaluate(
dsl.timestamp(ref("timestamp", STRING)))
.shouldMatch(new ExprTimestampValue("1984-03-17 22:16:42")
.timestampValue().toEpochMilli());
}

private ExprScriptAssertion assertThat() {
return new ExprScriptAssertion(lookup, leafLookup, context);
}
Expand Down

0 comments on commit 3eba57e

Please # to comment.