Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix assertion error when using passthrough with contradicting fields #110326

Merged

Conversation

felixbarny
Copy link
Member

I've added a yml rest test that reproduces an assertion error when using the passthrough field type with conflicts on multiple levels with a top-level match_only_text field.

[2024-07-01T03:48:58,737][ERROR][o.e.b.ElasticsearchUncaughtExceptionHandler] [test-cluster-1] fatal error in thread [elasticsearch[test-cluster-1][write][T#5]], exiting java.lang.AssertionError: Field body should not have docvalues
	at [email protected]/org.elasticsearch.index.mapper.FieldNamesFieldMapper.addFieldNames(FieldNamesFieldMapper.java:177)
	at [email protected]/org.elasticsearch.index.mapper.DocumentParserContext.addToFieldNames(DocumentParserContext.java:292)
	at [email protected]/org.elasticsearch.index.mapper.extras.MatchOnlyTextFieldMapper.parseCreateField(MatchOnlyTextFieldMapper.java:421)
	at [email protected]/org.elasticsearch.index.mapper.FieldMapper.parse(FieldMapper.java:185)
	at [email protected]/org.elasticsearch.index.mapper.DocumentParser.parseObjectOrField(DocumentParser.java:450)
	at [email protected]/org.elasticsearch.index.mapper.DocumentParser.parseValue(DocumentParser.java:775)
	at [email protected]/org.elasticsearch.index.mapper.DocumentParser.innerParseObject(DocumentParser.java:368)
	at [email protected]/org.elasticsearch.index.mapper.DocumentParser.parseObjectOrNested(DocumentParser.java:319)
	at [email protected]/org.elasticsearch.index.mapper.DocumentParser.internalParseDocument(DocumentParser.java:143)
	at [email protected]/org.elasticsearch.index.mapper.DocumentParser.parseDocument(DocumentParser.java:88)
	at [email protected]/org.elasticsearch.index.mapper.DocumentMapper.parse(DocumentMapper.java:112)
	at [email protected]/org.elasticsearch.index.shard.IndexShard.prepareIndex(IndexShard.java:1038)
	at [email protected]/org.elasticsearch.index.shard.IndexShard.applyIndexOperation(IndexShard.java:979)
	at [email protected]/org.elasticsearch.index.shard.IndexShard.applyIndexOperationOnPrimary(IndexShard.java:923)
	at [email protected]/org.elasticsearch.action.bulk.TransportShardBulkAction.executeBulkItemRequest(TransportShardBulkAction.java:376)
	at [email protected]/org.elasticsearch.action.bulk.TransportShardBulkAction$2.doRun(TransportShardBulkAction.java:234)
	at [email protected]/org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:26)
	at [email protected]/org.elasticsearch.common.util.concurrent.TimedRunnable.doRun(TimedRunnable.java:33)
	at [email protected]/org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingAbstractRunnable.doRun(ThreadContext.java:984)
	at [email protected]/org.elasticsearch.common.util.concurrent.AbstractRunnable.run(AbstractRunnable.java:26)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1570)

When changing the top-level field to a different field type, for example, keyword or text, the assertion error disappears, but the ordering is not quite right. Note that sometimes the tests pass and sometimes they fail with the following message: Expected a list containing 0: expected "top-level" but was "resource". Seems like in certain cases, the passthrough field type overrides the top-level field. I'm not sure if the two issues are related or separate.

This is a dump of the search:

[{
  "stash" : {
    "body" : {
      "took" : 35,
      "timed_out" : false,
      "_shards" : {
        "total" : 1,
        "successful" : 1,
        "skipped" : 0,
        "failed" : 0
      },
      "hits" : {
        "total" : {
          "value" : 1,
          "relation" : "eq"
        },
        "max_score" : 1.0,
        "hits" : [
          {
            "_index" : ".ds-otel-2024.07.01-000001",
            "_id" : "IHWS-EudCPGf0ayGAAABilDXF2o",
            "_score" : 1.0,
            "_source" : {
              "@timestamp" : "2023-09-01T13:03:08.138Z",
              "attributes" : {
                "body" : "attribute"
              },
              "body" : "top-level",
              "metrics" : {
                "data" : 10
              },
              "resource" : {
                "attributes" : {
                  "body" : "resource"
                }
              },
              "scope" : {
                "attributes" : {
                  "body" : "scope"
                }
              }
            },
            "fields" : {
              "metrics.data" : [
                10
              ],
              "@timestamp" : [
                "2023-09-01T13:03:08.138Z"
              ],
              "data" : [
                10
              ],
              "scope.attributes.body" : [
                "scope"
              ],
              "attributes.body" : [
                "attribute"
              ],
              "resource.attributes.body" : [
                "resource"
              ],
              "body" : [
                "resource"
              ]
            }
          }
        ]
      }
    }
  }
}]

@felixbarny felixbarny added the :StorageEngine/Mapping The storage related side of mappings label Jul 1, 2024
@elasticsearchmachine elasticsearchmachine added external-contributor Pull request authored by a developer outside the Elasticsearch team v8.15.0 labels Jul 1, 2024
@felixbarny felixbarny changed the title Assertion error using passthrough and match_only_text Fix assertion error when using passthrough with contradicting fields Jul 1, 2024
@felixbarny felixbarny marked this pull request as ready for review July 1, 2024 11:35
@felixbarny felixbarny requested a review from kkrik-es July 1, 2024 11:35
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for catching and sending a fix promptly.. Tricky logic, the new version is much cleaner.

@felixbarny felixbarny added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Jul 1, 2024
@elasticsearchmachine elasticsearchmachine merged commit b5ec38e into elastic:main Jul 1, 2024
15 checks passed
@felixbarny felixbarny deleted the passthrough-priority-issues branch July 1, 2024 15:22
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
8.14 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 110326

@felixbarny
Copy link
Member Author

The way passthrough field types are created and how the priority is configured has significantly changed in 8.15 with #106829. Backporting this fix to 8.14.x doesn't make sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) backport pending external-contributor Pull request authored by a developer outside the Elasticsearch team >non-issue :StorageEngine/Mapping The storage related side of mappings Team:StorageEngine v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants