Skip to content

Commit

Permalink
Merge branch '8.x' into esql/backport_117878
Browse files Browse the repository at this point in the history
  • Loading branch information
luigidellaquila authored Dec 4, 2024
2 parents 6ff9714 + 9abeeea commit e53639a
Show file tree
Hide file tree
Showing 13 changed files with 206 additions and 30 deletions.
6 changes: 6 additions & 0 deletions docs/changelog/116953.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
pr: 116953
summary: Fix false positive date detection with trailing dot
area: Mapping
type: bug
issues:
- 116946
6 changes: 6 additions & 0 deletions docs/reference/esql/functions/description/categorize.asciidoc

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ repeatedly-dropped connections will severely affect its operation.
The connections from the elected master node to every other node in the cluster
are particularly important. The elected master never spontaneously closes its
outbound connections to other nodes. Similarly, once an inbound connection is
fully established, a node never spontaneously it unless the node is shutting
fully established, a node never spontaneously closes it unless the node is shutting
down.

If you see a node unexpectedly leave the cluster with the `disconnected`
Expand Down
3 changes: 0 additions & 3 deletions muted-tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -326,9 +326,6 @@ tests:
- class: org.elasticsearch.xpack.searchablesnapshots.RetrySearchIntegTests
method: testRetryPointInTime
issue: https://github.com/elastic/elasticsearch/issues/117116
- class: org.elasticsearch.oldrepos.OldRepositoryAccessIT
method: testOldRepoAccess
issue: https://github.com/elastic/elasticsearch/issues/115631
- class: org.elasticsearch.xpack.inference.InferenceRestIT
method: test {p0=inference/40_semantic_text_query/Query a field that uses the default ELSER 2 endpoint}
issue: https://github.com/elastic/elasticsearch/issues/117027
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,10 @@ public long getFrom(TemporalAccessor temporal) {
.toFormatter(Locale.ROOT);

// this supports milliseconds ending in dot
private static final DateTimeFormatter MILLISECONDS_FORMATTER2 = new DateTimeFormatterBuilder().append(MILLISECONDS_FORMATTER1)
private static final DateTimeFormatter MILLISECONDS_FORMATTER2 = new DateTimeFormatterBuilder().optionalStart()
.appendText(NEGATIVE_SIGN_FIELD, Map.of(-1L, "-")) // field is only created in the presence of a '-' char.
.optionalEnd()
.appendValue(UNSIGNED_MILLIS, 1, 19, SignStyle.NOT_NEGATIVE)
.appendLiteral('.')
.toFormatter(Locale.ROOT);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.util.List;

public class DocumentMapper {
static final NodeFeature INDEX_SORTING_ON_NESTED = new NodeFeature("mapper.index_sorting_on_nested");

private final String type;
private final CompressedXContent mappingSource;
private final MappingLookup mappingLookup;
Expand All @@ -29,8 +31,6 @@ public class DocumentMapper {
private final IndexVersion indexVersion;
private final Logger logger;

static final NodeFeature INDEX_SORTING_ON_NESTED = new NodeFeature("mapper.index_sorting_on_nested");

/**
* Create a new {@link DocumentMapper} that holds empty mappings.
* @param mapperService the mapper service that holds the needed components
Expand Down Expand Up @@ -72,9 +72,11 @@ public static DocumentMapper createEmpty(MapperService mapperService) {
: "provided source [" + source + "] differs from mapping [" + mapping.toCompressedXContent() + "]";
}

private void maybeLogDebug(Exception ex) {
private void maybeLog(Exception ex) {
if (logger.isDebugEnabled()) {
logger.debug("Error while parsing document: " + ex.getMessage(), ex);
} else if (IntervalThrottler.DOCUMENT_PARSING_FAILURE.accept()) {
logger.error("Error while parsing document: " + ex.getMessage(), ex);
}
}

Expand Down Expand Up @@ -125,7 +127,7 @@ public ParsedDocument parse(SourceToParse source) throws DocumentParsingExceptio
try {
return documentParser.parseDocument(source, mappingLookup);
} catch (Exception e) {
maybeLogDebug(e);
maybeLog(e);
throw e;
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.index.mapper;

import java.util.concurrent.atomic.AtomicBoolean;

/**
* Throttles tracked operations based on a time interval, restricting them to 1 per N seconds.
*/
enum IntervalThrottler {
DOCUMENT_PARSING_FAILURE(60);

static final int MILLISECONDS_IN_SECOND = 1000;

private final Acceptor acceptor;

IntervalThrottler(long intervalSeconds) {
acceptor = new Acceptor(intervalSeconds * MILLISECONDS_IN_SECOND);
}

/**
* @return true if the operation gets accepted, false if throttled.
*/
boolean accept() {
return acceptor.accept();
}

// Defined separately for testing.
static class Acceptor {
private final long intervalMillis;
private final AtomicBoolean lastAcceptedGuard = new AtomicBoolean(false);
private volatile long lastAcceptedTimeMillis = 0;

Acceptor(long intervalMillis) {
this.intervalMillis = intervalMillis;
}

boolean accept() {
final long now = System.currentTimeMillis();
// Check without guarding first, to reduce contention.
if (now - lastAcceptedTimeMillis > intervalMillis) {
// Check if another concurrent operation succeeded.
if (lastAcceptedGuard.compareAndSet(false, true)) {
try {
// Repeat check under guard protection, so that only one message gets written per interval.
if (now - lastAcceptedTimeMillis > intervalMillis) {
lastAcceptedTimeMillis = now;
return true;
}
} finally {
// Reset guard.
lastAcceptedGuard.set(false);
}
}
}
return false;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,17 @@ public void testEpochMillisParser() {
assertThat(formatter.format(instant), is("-0.12345"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
Instant instant = Instant.from(formatter.parse("12345."));
assertThat(instant.getEpochSecond(), is(12L));
assertThat(instant.getNano(), is(345_000_000));
assertThat(formatter.format(instant), is("12345"));
assertThat(Instant.from(formatter.parse(formatter.format(instant))), is(instant));
}
{
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> formatter.parse("12345.0."));
assertThat(e.getMessage(), is("failed to parse date field [12345.0.] with format [epoch_millis]"));
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,18 @@

package org.elasticsearch.index.mapper;

import org.apache.logging.log4j.Level;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.core.LogEvent;
import org.apache.lucene.analysis.Analyzer;
import org.apache.lucene.analysis.core.KeywordAnalyzer;
import org.apache.lucene.analysis.core.WhitespaceAnalyzer;
import org.apache.lucene.analysis.standard.StandardAnalyzer;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.compress.CompressedXContent;
import org.elasticsearch.common.logging.Loggers;
import org.elasticsearch.common.logging.MockAppender;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.index.IndexSettings;
import org.elasticsearch.index.IndexVersion;
Expand Down Expand Up @@ -493,4 +499,35 @@ public void testDeeplyNestedMapping() throws Exception {
}
}
}

public void testParsingErrorLogging() throws Exception {
MockAppender appender = new MockAppender("mock_appender");
appender.start();
Logger testLogger = LogManager.getLogger(DocumentMapper.class);
Loggers.addAppender(testLogger, appender);
Level originalLogLevel = testLogger.getLevel();
Loggers.setLevel(testLogger, Level.ERROR);

try {
DocumentMapper doc = createDocumentMapper(mapping(b -> b.startObject("value").field("type", "integer").endObject()));

DocumentParsingException e = expectThrows(
DocumentParsingException.class,
() -> doc.parse(source(b -> b.field("value", "foo")))
);
assertThat(e.getMessage(), containsString("failed to parse field [value] of type [integer] in document with id '1'"));
LogEvent event = appender.getLastEventAndReset();
if (event != null) {
assertThat(event.getMessage().getFormattedMessage(), containsString(e.getMessage()));
}

e = expectThrows(DocumentParsingException.class, () -> doc.parse(source(b -> b.field("value", "foo"))));
assertThat(e.getMessage(), containsString("failed to parse field [value] of type [integer] in document with id '1'"));
assertThat(appender.getLastEventAndReset(), nullValue());
} finally {
Loggers.setLevel(testLogger, originalLogLevel);
Loggers.removeAppender(testLogger, appender);
appender.stop();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -597,6 +597,23 @@ private void doTestDefaultFloatingPointMappings(DocumentMapper mapper, XContentB
assertThat(((FieldMapper) update.getRoot().getMapper("quux")).fieldType().typeName(), equalTo("float"));
}

public void testDateDetectionEnabled() throws Exception {
MapperService mapperService = createMapperService(topMapping(b -> b.field("date_detection", true)));

ParsedDocument doc = mapperService.documentMapper().parse(source(b -> {
b.field("date", "2024-11-18");
b.field("no_date", "128.0.");
}));
assertNotNull(doc.dynamicMappingsUpdate());
merge(mapperService, dynamicMapping(doc.dynamicMappingsUpdate()));

Mapper mapper = mapperService.documentMapper().mappers().getMapper("date");
assertThat(mapper.typeName(), equalTo("date"));

mapper = mapperService.documentMapper().mappers().getMapper("no_date");
assertThat(mapper.typeName(), equalTo("text"));
}

public void testNumericDetectionEnabled() throws Exception {
MapperService mapperService = createMapperService(topMapping(b -> b.field("numeric_detection", true)));

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.index.mapper;

import org.elasticsearch.test.ESTestCase;

public class IntervalThrottlerTests extends ESTestCase {

public void testThrottling() throws Exception {
var throttler = new IntervalThrottler.Acceptor(10);
assertTrue(throttler.accept());
assertFalse(throttler.accept());
assertFalse(throttler.accept());

Thread.sleep(20);
assertTrue(throttler.accept());
assertFalse(throttler.accept());
assertFalse(throttler.accept());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@
import org.apache.http.ssl.SSLContextBuilder;
import org.apache.http.ssl.SSLContexts;
import org.apache.http.util.EntityUtils;
import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.elasticsearch.Build;
import org.elasticsearch.TransportVersion;
import org.elasticsearch.TransportVersions;
Expand Down Expand Up @@ -158,6 +160,8 @@ public abstract class ESRestTestCase extends ESTestCase {

private static final Pattern SEMANTIC_VERSION_PATTERN = Pattern.compile("^(\\d+\\.\\d+\\.\\d+)\\D?.*");

private static final Logger SUITE_LOGGER = LogManager.getLogger(ESRestTestCase.class);

/**
* Convert the entity from a {@link Response} into a map of maps.
* Consumes the underlying HttpEntity, releasing any resources it may be holding.
Expand Down Expand Up @@ -1171,7 +1175,13 @@ protected static void wipeAllIndices(boolean preserveSecurityIndices) throws IOE
}
final Request deleteRequest = new Request("DELETE", Strings.collectionToCommaDelimitedString(indexPatterns));
deleteRequest.addParameter("expand_wildcards", "open,closed" + (includeHidden ? ",hidden" : ""));
deleteRequest.setOptions(deleteRequest.getOptions().toBuilder().setWarningsHandler(ignoreAsyncSearchWarning()).build());

// If system index warning, ignore but log
// See: https://github.com/elastic/elasticsearch/issues/117099
// and: https://github.com/elastic/elasticsearch/issues/115809
deleteRequest.setOptions(
RequestOptions.DEFAULT.toBuilder().setWarningsHandler(ESRestTestCase::ignoreSystemIndexAccessWarnings)
);
final Response response = adminClient().performRequest(deleteRequest);
try (InputStream is = response.getEntity().getContent()) {
assertTrue((boolean) XContentHelper.convertToMap(XContentType.JSON.xContent(), is, true).get("acknowledged"));
Expand All @@ -1184,28 +1194,16 @@ protected static void wipeAllIndices(boolean preserveSecurityIndices) throws IOE
}
}

// Make warnings handler that ignores the .async-search warning since .async-search may randomly appear when async requests are slow
// See: https://github.com/elastic/elasticsearch/issues/117099
protected static WarningsHandler ignoreAsyncSearchWarning() {
return new WarningsHandler() {
@Override
public boolean warningsShouldFailRequest(List<String> warnings) {
if (warnings.isEmpty()) {
return false;
}
return warnings.equals(
List.of(
"this request accesses system indices: [.async-search], "
+ "but in a future major version, direct access to system indices will be prevented by default"
)
) == false;
private static boolean ignoreSystemIndexAccessWarnings(List<String> warnings) {
for (String warning : warnings) {
if (warning.startsWith("this request accesses system indices:")) {
SUITE_LOGGER.warn("Ignoring system index access warning during test cleanup: {}", warning);
} else {
return true;
}
}

@Override
public String toString() {
return "ignore .async-search warning";
}
};
return false;
}

protected static void wipeDataStreams() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,12 @@ public class Categorize extends GroupingFunction implements Validatable {
@FunctionInfo(
returnType = "keyword",
description = "Groups text messages into categories of similarly formatted text values.",
detailedDescription = """
`CATEGORIZE` has the following limitations:
* can't be used within other expressions
* can't be used with multiple groupings
* can't be used or referenced within aggregate functions""",
examples = {
@Example(
file = "docs",
Expand Down

0 comments on commit e53639a

Please sign in to comment.