From 61a0c60116a872e8495e19079328ee2182ed71b8 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 20 Oct 2020 15:52:13 +0200 Subject: [PATCH 1/4] add support for unsigned_long, which required a change in writing out integer results properly, because coerce is not supported for unsigned_long fixes #63871 --- .../integration/TransformPivotRestIT.java | 12 ++-- .../continuous/DateHistogramGroupByIT.java | 12 ++-- .../DateHistogramGroupByOtherTimeFieldIT.java | 12 ++-- .../continuous/TermsGroupByIT.java | 11 ++-- .../continuous/TransformContinuousIT.java | 2 +- .../pivot/AggregationResultUtils.java | 3 +- .../transforms/pivot/SchemaUtil.java | 59 +++++++++++++++---- .../pivot/AggregationResultUtilsTests.java | 12 +++- .../AggregationSchemaAndResultTests.java | 16 +++-- 9 files changed, 95 insertions(+), 44 deletions(-) diff --git a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformPivotRestIT.java b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformPivotRestIT.java index e100fc6625743..e52717dda65cf 100644 --- a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformPivotRestIT.java +++ b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/TransformPivotRestIT.java @@ -557,15 +557,15 @@ public void testContinuousPivotHistogram() throws Exception { Map searchResult = getAsMap(transformIndex + "/_search?q=every_2:0.0"); assertEquals(1, XContentMapValues.extractValue("hits.total.value", searchResult)); - assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(19.0)); + assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(19)); searchResult = getAsMap(transformIndex + "/_search?q=every_2:2.0"); assertEquals(1, XContentMapValues.extractValue("hits.total.value", searchResult)); - assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(27.0)); + assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(27)); searchResult = getAsMap(transformIndex + "/_search?q=every_2:4.0"); assertEquals(1, XContentMapValues.extractValue("hits.total.value", searchResult)); - assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(27.0)); + assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(27)); final StringBuilder bulk = new StringBuilder(); @@ -609,15 +609,15 @@ public void testContinuousPivotHistogram() throws Exception { searchResult = getAsMap(transformIndex + "/_search?q=every_2:0.0"); assertEquals(1, XContentMapValues.extractValue("hits.total.value", searchResult)); - assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(19.0)); + assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(19)); searchResult = getAsMap(transformIndex + "/_search?q=every_2:2.0"); assertEquals(1, XContentMapValues.extractValue("hits.total.value", searchResult)); - assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(30.0)); + assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(30)); searchResult = getAsMap(transformIndex + "/_search?q=every_2:4.0"); assertEquals(1, XContentMapValues.extractValue("hits.total.value", searchResult)); - assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(27.0)); + assertThat(((List) XContentMapValues.extractValue("hits.hits._source.user_dc", searchResult)).get(0), equalTo(27)); } diff --git a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByIT.java b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByIT.java index f26279602dc78..4364355d2afa8 100644 --- a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByIT.java +++ b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByIT.java @@ -130,8 +130,8 @@ public void testIteration(int iteration) throws IOException { ); assertThat( "Doc count did not match, source: " + source + ", expected: " + bucket.getDocCount() + ", iteration: " + iteration, - XContentMapValues.extractValue("count", source), - equalTo(Double.valueOf(bucket.getDocCount())) + ((Integer) XContentMapValues.extractValue("count", source)).longValue(), + equalTo(bucket.getDocCount()) ); // transform should only rewrite documents that require it @@ -146,9 +146,11 @@ public void testIteration(int iteration) throws IOException { // we use a fixed_interval of `1s`, the transform runs every `1s` so it the bucket might be recalculated at the next run // but // should NOT be recalculated for the 2nd/3rd/... run - Double.valueOf((Integer) XContentMapValues.extractValue(INGEST_RUN_FIELD, source)) - (Double) XContentMapValues - .extractValue(MAX_RUN_FIELD, source), - is(lessThanOrEqualTo(1.0)) + (Integer) XContentMapValues.extractValue(INGEST_RUN_FIELD, source) - (Integer) XContentMapValues.extractValue( + MAX_RUN_FIELD, + source + ), + is(lessThanOrEqualTo(1)) ); } } diff --git a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java index 938834887afc8..99f55af067d54 100644 --- a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java +++ b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java @@ -138,8 +138,8 @@ private void assertResultsGroupByDateHistogram(int iteration, SearchResponse res ); assertThat( "Doc count did not match, source: " + source + ", expected: " + bucket.getDocCount() + ", iteration: " + iteration, - XContentMapValues.extractValue("count", source), - equalTo(Double.valueOf(bucket.getDocCount())) + ((Integer) XContentMapValues.extractValue("count", source)).longValue(), + equalTo(bucket.getDocCount()) ); // transform should only rewrite documents that require it @@ -152,9 +152,11 @@ private void assertResultsGroupByDateHistogram(int iteration, SearchResponse res + iteration, // we use a fixed_interval of `1s`, the transform runs every `1s`, a bucket might be recalculated at the next run // but should NOT be recalculated for the 2nd/3rd/... run - Double.valueOf((Integer) XContentMapValues.extractValue(INGEST_RUN_FIELD, source)) - (Double) XContentMapValues - .extractValue(MAX_RUN_FIELD, source), - is(lessThanOrEqualTo(1.0)) + (Integer) XContentMapValues.extractValue(INGEST_RUN_FIELD, source) - (Integer) XContentMapValues.extractValue( + MAX_RUN_FIELD, + source + ), + is(lessThanOrEqualTo(1)) ); } diff --git a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TermsGroupByIT.java b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TermsGroupByIT.java index dd17ebfbc89cc..841463234bd7f 100644 --- a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TermsGroupByIT.java +++ b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TermsGroupByIT.java @@ -60,7 +60,7 @@ public TransformConfig createConfig() { AggregatorFactories.Builder aggregations = new AggregatorFactories.Builder(); addCommonAggregations(aggregations); - aggregations.addAggregator(AggregationBuilders.max("metric.avg").field("metric")); + aggregations.addAggregator(AggregationBuilders.avg("metric.avg").field("metric")); pivotConfigBuilder.setAggregations(aggregations); transformConfigBuilder.setPivotConfig(pivotConfigBuilder.build()); @@ -83,7 +83,7 @@ public void testIteration(int iteration) throws IOException { // missing_bucket produces `null`, we can't use `null` in aggs, so we have to use a magic value, see gh#60043 terms.missing(MISSING_BUCKET_KEY); } - terms.subAggregation(AggregationBuilders.max("metric.avg").field("metric")); + terms.subAggregation(AggregationBuilders.avg("metric.avg").field("metric")); sourceBuilderSource.aggregation(terms); searchRequestSource.source(sourceBuilderSource); SearchResponse responseSource = search(searchRequestSource); @@ -129,8 +129,8 @@ public void testIteration(int iteration) throws IOException { ); assertThat( "Doc count did not match, source: " + source + ", expected: " + bucket.getDocCount() + ", iteration: " + iteration, - XContentMapValues.extractValue("count", source), - equalTo(Double.valueOf(bucket.getDocCount())) + ((Integer) XContentMapValues.extractValue("count", source)).longValue(), + equalTo(bucket.getDocCount()) ); SingleValue avgAgg = (SingleValue) bucket.getAggregations().get("metric.avg"); @@ -154,8 +154,7 @@ public void testIteration(int iteration) throws IOException { + iteration + " full source: " + source, - // TODO: aggs return double for MAX_RUN_FIELD, although it is an integer - Double.valueOf((Integer) XContentMapValues.extractValue(INGEST_RUN_FIELD, source)), + XContentMapValues.extractValue(INGEST_RUN_FIELD, source), equalTo(XContentMapValues.extractValue(MAX_RUN_FIELD, source)) ); } diff --git a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TransformContinuousIT.java b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TransformContinuousIT.java index 4f298a71db3fe..8b011ecdabe46 100644 --- a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TransformContinuousIT.java +++ b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/TransformContinuousIT.java @@ -316,7 +316,7 @@ private void putIndex(String indexName, String dateType, boolean isDataStream) t .field("type", "keyword") .endObject() .startObject("metric") - .field("type", "integer") + .field("type", randomFrom("integer", "long", "unsigned_long")) .endObject() .startObject("location") .field("type", "geo_point") diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java index fbdffd1ec86ab..427a5739b8e7a 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java @@ -40,6 +40,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static org.elasticsearch.xpack.transform.transforms.pivot.SchemaUtil.convertToIntegerTypeIfNeeded; import static org.elasticsearch.xpack.transform.transforms.pivot.SchemaUtil.isNumericType; public final class AggregationResultUtils { @@ -198,7 +199,7 @@ public Object value(Aggregation agg, Map fieldTypeMap, String lo // If the type is numeric or if the formatted string is the same as simply making the value a string, // gather the `value` type, otherwise utilize `getValueAsString` so we don't lose formatted outputs. if (isNumericType(fieldType) || aggregation.getValueAsString().equals(String.valueOf(aggregation.value()))) { - return aggregation.value(); + return convertToIntegerTypeIfNeeded(fieldType, aggregation.value()); } else { return aggregation.getValueAsString(); } diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java index 7bbf1d2faddff..d8a09d8f694a5 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java @@ -23,30 +23,59 @@ import org.elasticsearch.xpack.core.ClientHelper; import org.elasticsearch.xpack.core.transform.transforms.pivot.PivotConfig; +import java.math.BigDecimal; import java.util.HashMap; import java.util.Map; import java.util.Objects; -import java.util.Set; import java.util.stream.Collectors; import java.util.stream.Stream; public final class SchemaUtil { private static final Logger logger = LogManager.getLogger(SchemaUtil.class); - // Full collection of numeric field type strings - private static final Set NUMERIC_FIELD_MAPPER_TYPES; + // Full collection of numeric field type strings and whether they are floating point or not + private static final Map NUMERIC_FIELD_MAPPER_TYPES; static { - Set types = Stream.of(NumberFieldMapper.NumberType.values()) - .map(NumberFieldMapper.NumberType::typeName) - .collect(Collectors.toSet()); - types.add("scaled_float"); // have to add manually since scaled_float is in a module + Map types = Stream.of(NumberFieldMapper.NumberType.values()) + .collect(Collectors.toMap(t -> t.typeName(), t -> t.numericType().isFloatingPoint())); + + // have to add manually since they are in a module + types.put("scaled_float", true); + types.put("unsigned_long", false); NUMERIC_FIELD_MAPPER_TYPES = types; } private SchemaUtil() {} public static boolean isNumericType(String type) { - return type != null && NUMERIC_FIELD_MAPPER_TYPES.contains(type); + return type != null && NUMERIC_FIELD_MAPPER_TYPES.containsKey(type); + } + + /** + * Convert a numeric value to an integer if it's not a floating point number. + * + * Implementation decision: We do not care about the concrete type, but only if its floating point or not. + * Further checks (e.g. range) are done at indexing. + * + * If type is floating point and value could be an integer (ends with `.0`), we still preserve `.0` in case + * the destination index uses dynamic mappings as well as being json friendly. + * + * @param type the type of the value according to the schema we know + * @param value the value as double (aggs return double for everything) + * @return value if its floating point, a integer + */ + public static Object convertToIntegerTypeIfNeeded(String type, double value) { + if (NUMERIC_FIELD_MAPPER_TYPES.getOrDefault(type, true) == false) { + assert value % 1 == 0; + if (value < Long.MAX_VALUE) { + return (long) value; + } + + // special case for unsigned long + return BigDecimal.valueOf(value).toBigInteger(); + } + + return value; } /** @@ -188,8 +217,11 @@ private static Map resolveMappings( } else if (destinationMapping != null) { targetMapping.put(targetFieldName, destinationMapping); } else { - logger.warn("Failed to deduce mapping for [{}], fall back to dynamic mapping. " + - "Create the destination index with complete mappings first to avoid deducing the mappings", targetFieldName); + logger.warn( + "Failed to deduce mapping for [{}], fall back to dynamic mapping. " + + "Create the destination index with complete mappings first to avoid deducing the mappings", + targetFieldName + ); } }); @@ -199,8 +231,11 @@ private static Map resolveMappings( if (destinationMapping != null) { targetMapping.put(targetFieldName, destinationMapping); } else { - logger.warn("Failed to deduce mapping for [{}], fall back to keyword. " + - "Create the destination index with complete mappings first to avoid deducing the mappings", targetFieldName); + logger.warn( + "Failed to deduce mapping for [{}], fall back to keyword. " + + "Create the destination index with complete mappings first to avoid deducing the mappings", + targetFieldName + ); targetMapping.put(targetFieldName, KeywordFieldMapper.CONTENT_TYPE); } }); diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtilsTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtilsTests.java index 0040d5b7c7119..49ff7ad0a35fd 100644 --- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtilsTests.java +++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtilsTests.java @@ -654,6 +654,12 @@ public void testSingleValueAggExtractor() { AggregationResultUtils.getExtractor(agg).value(agg, Collections.singletonMap("metric", "string"), ""), equalTo("one_hundred") ); + + agg = createSingleMetricAgg("metric", 100.0, "one_hundred"); + assertThat( + AggregationResultUtils.getExtractor(agg).value(agg, Collections.singletonMap("metric", "unsigned_long"), ""), + equalTo(100L) + ); } private ScriptedMetric createScriptedMetric(Object returnValue) { @@ -836,7 +842,7 @@ public void testSingleBucketAggExtractor() { ); assertThat( AggregationResultUtils.getExtractor(agg).value(agg, asStringMap("sba2.sub1", "long", "sba2.sub2", "float"), ""), - equalTo(asMap("sub1", 100.0, "sub2", 33.33)) + equalTo(asMap("sub1", 100L, "sub2", 33.33)) ); agg = createSingleBucketAgg( @@ -848,7 +854,7 @@ public void testSingleBucketAggExtractor() { ); assertThat( AggregationResultUtils.getExtractor(agg).value(agg, asStringMap("sba3.sub1", "long", "sba3.sub2", "double"), ""), - equalTo(asMap("sub1", 100.0, "sub2", 33.33, "sub3", 42L)) + equalTo(asMap("sub1", 100L, "sub2", 33.33, "sub3", 42L)) ); agg = createSingleBucketAgg( @@ -861,7 +867,7 @@ public void testSingleBucketAggExtractor() { assertThat( AggregationResultUtils.getExtractor(agg) .value(agg, asStringMap("sba4.sub3.subsub1", "double", "sba4.sub2", "float", "sba4.sub1", "long"), ""), - equalTo(asMap("sub1", 100.0, "sub2", 33.33, "sub3", asMap("subsub1", 11.1))) + equalTo(asMap("sub1", 100L, "sub2", 33.33, "sub3", asMap("subsub1", 11.1))) ); } diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationSchemaAndResultTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationSchemaAndResultTests.java index a2e811993e522..613d049b48319 100644 --- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationSchemaAndResultTests.java +++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationSchemaAndResultTests.java @@ -127,8 +127,11 @@ public void testBasic() throws InterruptedException { AggregationConfig aggregationConfig = new AggregationConfig(Collections.emptyMap(), aggs); GroupConfig groupConfig = GroupConfigTests.randomGroupConfig(); PivotConfig pivotConfig = new PivotConfig(groupConfig, aggregationConfig, null); - long numGroupsWithoutScripts = groupConfig.getGroups().values().stream() - .filter(singleGroupSource -> singleGroupSource.getScriptConfig() == null).count(); + long numGroupsWithoutScripts = groupConfig.getGroups() + .values() + .stream() + .filter(singleGroupSource -> singleGroupSource.getScriptConfig() == null) + .count(); this.>assertAsync( listener -> SchemaUtil.deduceMappings(client, pivotConfig, new String[] { "source-index" }, listener), @@ -191,8 +194,11 @@ public void testNested() throws InterruptedException { AggregationConfig aggregationConfig = new AggregationConfig(Collections.emptyMap(), aggs); GroupConfig groupConfig = GroupConfigTests.randomGroupConfig(); PivotConfig pivotConfig = new PivotConfig(groupConfig, aggregationConfig, null); - long numGroupsWithoutScripts = groupConfig.getGroups().values().stream() - .filter(singleGroupSource -> singleGroupSource.getScriptConfig() == null).count(); + long numGroupsWithoutScripts = groupConfig.getGroups() + .values() + .stream() + .filter(singleGroupSource -> singleGroupSource.getScriptConfig() == null) + .count(); this.>assertAsync( listener -> SchemaUtil.deduceMappings(client, pivotConfig, new String[] { "source-index" }, listener), @@ -219,7 +225,7 @@ public void testNested() throws InterruptedException { 23144, AggregationResultUtilsTests.createSingleMetricAgg("max_drinks_2", 45.0, "forty_five") ); - assertThat(AggregationResultUtils.getExtractor(agg).value(agg, mappings, ""), equalTo(asMap("max_drinks_2", 45.0))); + assertThat(AggregationResultUtils.getExtractor(agg).value(agg, mappings, ""), equalTo(asMap("max_drinks_2", 45L))); agg = AggregationResultUtilsTests.createSingleBucketAgg( "filter_3", From 2f29642dcdc31189e713f71869f3bad74bbeef5c Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 20 Oct 2020 16:46:19 +0200 Subject: [PATCH 2/4] add tests --- .../DateHistogramGroupByOtherTimeFieldIT.java | 12 +++++++----- .../transforms/pivot/SchemaUtilTests.java | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java index 99f55af067d54..65deb06b88a37 100644 --- a/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java +++ b/x-pack/plugin/transform/qa/single-node-tests/src/javaRestTest/java/org/elasticsearch/xpack/transform/integration/continuous/DateHistogramGroupByOtherTimeFieldIT.java @@ -198,8 +198,8 @@ private void assertResultsGroupByDateHistogramAndTerms(int iteration, SearchResp ); assertThat( "Doc count did not match, source: " + source + ", expected: " + bucket.get("count") + ", iteration: " + iteration, - XContentMapValues.extractValue("count", source), - equalTo(Double.valueOf(((Long) bucket.get("count")))) + ((Integer) XContentMapValues.extractValue("count", source)).longValue(), + equalTo(bucket.get("count")) ); assertThat( "Term did not match, source: " + source + ", expected: " + bucket.get("event") + ", iteration: " + iteration, @@ -217,9 +217,11 @@ private void assertResultsGroupByDateHistogramAndTerms(int iteration, SearchResp + iteration, // we use a fixed_interval of `1s`, the transform runs every `1s`, a bucket might be recalculated at the next run // but should NOT be recalculated for the 2nd/3rd/... run - Double.valueOf((Integer) XContentMapValues.extractValue(INGEST_RUN_FIELD, source)) - (Double) XContentMapValues - .extractValue(MAX_RUN_FIELD, source), - is(lessThanOrEqualTo(1.0)) + (Integer) XContentMapValues.extractValue(INGEST_RUN_FIELD, source) - (Integer) XContentMapValues.extractValue( + MAX_RUN_FIELD, + source + ), + is(lessThanOrEqualTo(1)) ); } assertFalse(sourceIterator.hasNext()); diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java index 8dd8a5f816221..62e49bf1f70a8 100644 --- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java +++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java @@ -8,9 +8,12 @@ import org.elasticsearch.test.ESTestCase; +import java.math.BigInteger; import java.util.HashMap; import java.util.Map; +import static org.hamcrest.CoreMatchers.instanceOf; + public class SchemaUtilTests extends ESTestCase { public void testInsertNestedObjectMappings() { @@ -54,4 +57,18 @@ public void testInsertNestedObjectMappings() { assertFalse(fieldMappings.containsKey("")); } + public void testConvertToIntegerTypeIfNeeded() { + assertEquals(33L, SchemaUtil.convertToIntegerTypeIfNeeded("unsigned_long", 33.0)); + assertEquals(33L, SchemaUtil.convertToIntegerTypeIfNeeded("long", 33.0)); + assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded("double", 33.0)); + assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded("half_float", 33.0)); + assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded("unknown", 33.0)); + assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded(null, 33.0)); + + Object value = SchemaUtil.convertToIntegerTypeIfNeeded("unsigned_long", 1.8446744073709551615E19); + assertThat(value, instanceOf(BigInteger.class)); + + assertEquals(new BigInteger("18446744073709551615").doubleValue(), ((BigInteger) value).doubleValue(), 0.0); + } + } From 6a54a508e4616a177b1112ecbcfd83cf23420cf4 Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 20 Oct 2020 17:46:24 +0200 Subject: [PATCH 3/4] rename method and update docstrings --- .../transforms/pivot/AggregationResultUtils.java | 4 ++-- .../transform/transforms/pivot/SchemaUtil.java | 8 ++++---- .../transforms/pivot/SchemaUtilTests.java | 14 +++++++------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java index 427a5739b8e7a..c5bde60717123 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/AggregationResultUtils.java @@ -40,7 +40,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; -import static org.elasticsearch.xpack.transform.transforms.pivot.SchemaUtil.convertToIntegerTypeIfNeeded; +import static org.elasticsearch.xpack.transform.transforms.pivot.SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt; import static org.elasticsearch.xpack.transform.transforms.pivot.SchemaUtil.isNumericType; public final class AggregationResultUtils { @@ -199,7 +199,7 @@ public Object value(Aggregation agg, Map fieldTypeMap, String lo // If the type is numeric or if the formatted string is the same as simply making the value a string, // gather the `value` type, otherwise utilize `getValueAsString` so we don't lose formatted outputs. if (isNumericType(fieldType) || aggregation.getValueAsString().equals(String.valueOf(aggregation.value()))) { - return convertToIntegerTypeIfNeeded(fieldType, aggregation.value()); + return dropFloatingPointComponentIfTypeRequiresIt(fieldType, aggregation.value()); } else { return aggregation.getValueAsString(); } diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java index d8a09d8f694a5..cdd6bbb23e1ab 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java @@ -52,19 +52,19 @@ public static boolean isNumericType(String type) { } /** - * Convert a numeric value to an integer if it's not a floating point number. + * Convert a numeric value to a whole number if it's not a floating point number. * * Implementation decision: We do not care about the concrete type, but only if its floating point or not. * Further checks (e.g. range) are done at indexing. * - * If type is floating point and value could be an integer (ends with `.0`), we still preserve `.0` in case + * If type is floating point but ends with `.0`, we still preserve `.0` in case * the destination index uses dynamic mappings as well as being json friendly. * * @param type the type of the value according to the schema we know * @param value the value as double (aggs return double for everything) - * @return value if its floating point, a integer + * @return value if its floating point, long if < Long.MAX_VALUE, BigInteger if value >= Long.MAX_VALUE */ - public static Object convertToIntegerTypeIfNeeded(String type, double value) { + public static Object dropFloatingPointComponentIfTypeRequiresIt(String type, double value) { if (NUMERIC_FIELD_MAPPER_TYPES.getOrDefault(type, true) == false) { assert value % 1 == 0; if (value < Long.MAX_VALUE) { diff --git a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java index 62e49bf1f70a8..3db24778e4fa4 100644 --- a/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java +++ b/x-pack/plugin/transform/src/test/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtilTests.java @@ -58,14 +58,14 @@ public void testInsertNestedObjectMappings() { } public void testConvertToIntegerTypeIfNeeded() { - assertEquals(33L, SchemaUtil.convertToIntegerTypeIfNeeded("unsigned_long", 33.0)); - assertEquals(33L, SchemaUtil.convertToIntegerTypeIfNeeded("long", 33.0)); - assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded("double", 33.0)); - assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded("half_float", 33.0)); - assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded("unknown", 33.0)); - assertEquals(33.0, SchemaUtil.convertToIntegerTypeIfNeeded(null, 33.0)); + assertEquals(33L, SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt("unsigned_long", 33.0)); + assertEquals(33L, SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt("long", 33.0)); + assertEquals(33.0, SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt("double", 33.0)); + assertEquals(33.0, SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt("half_float", 33.0)); + assertEquals(33.0, SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt("unknown", 33.0)); + assertEquals(33.0, SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt(null, 33.0)); - Object value = SchemaUtil.convertToIntegerTypeIfNeeded("unsigned_long", 1.8446744073709551615E19); + Object value = SchemaUtil.dropFloatingPointComponentIfTypeRequiresIt("unsigned_long", 1.8446744073709551615E19); assertThat(value, instanceOf(BigInteger.class)); assertEquals(new BigInteger("18446744073709551615").doubleValue(), ((BigInteger) value).doubleValue(), 0.0); From 6044571faee9aab6f3a88546e4f693e92c0b422a Mon Sep 17 00:00:00 2001 From: Hendrik Muhs Date: Tue, 20 Oct 2020 17:57:48 +0200 Subject: [PATCH 4/4] fix doc string --- .../xpack/transform/transforms/pivot/SchemaUtil.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java index cdd6bbb23e1ab..0c839ba799564 100644 --- a/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java +++ b/x-pack/plugin/transform/src/main/java/org/elasticsearch/xpack/transform/transforms/pivot/SchemaUtil.java @@ -62,7 +62,7 @@ public static boolean isNumericType(String type) { * * @param type the type of the value according to the schema we know * @param value the value as double (aggs return double for everything) - * @return value if its floating point, long if < Long.MAX_VALUE, BigInteger if value >= Long.MAX_VALUE + * @return value if its floating point, long if value is smaller than Long.MAX_VALUE, BigInteger otherwise */ public static Object dropFloatingPointComponentIfTypeRequiresIt(String type, double value) { if (NUMERIC_FIELD_MAPPER_TYPES.getOrDefault(type, true) == false) {