Skip to content

Conversation

@flobernd
Copy link
Member

@flobernd flobernd commented Feb 3, 2025

Some types like e.g. TermQuery have a shortcut to a property with the FieldValue type.

According to the specification, FieldValue was allowed to contain UserDefinedValue/any. This complicates deserialization in the shortcut property case since we can't simply rely on the JSON token type (!= Object) to know that we are handling a shortcut property. While this can be worked around, I still started to check, if FieldValue should even be supposed to contain non-scalar values.

@pquentin checked the validation results and found 3 issues:

  1. ES|QL queries where params is typed as FieldValue[] but named parameters like {"n1": 300} are allowed.
  2. ignored_field_values in responses which can be a string (keyword in this example) or a dictionary (flat in this example)
  3. TermsQuery where additional properties are of type FieldValue[] but can be lists or dictionaries in NEST tests

Checking these cases shows the following:

  1. We know that ESQL params are currently not modelled in the best way possible.

    Proposal: Replace with UserDefinedValue until we fixed the core issue.

  2. https://www.elastic.co/guide/en/elasticsearch/reference/current/search-fields.html#retrieve-unmapped-fields
    According to this quote, we are currently using the wrong type as the returned values are not canonical field-values:

    The response will contain ignored field values under the ignored_field_values path. These values are retrieved from the document’s original JSON source and are raw so will not be formatted or treated in any way, unlike the successfully indexed fields which are returned in the fields section.

    Proposal: Replace with UserDefinedValue.

  3. The first test is wrong "error": true. The second test is syntactically valid, but according to another test @pquentin did, does not return semantically correct results.

    Conclusion: Remove/ignore these tests and continue to use FieldValue.

Request:

# Create an index
PUT /my-index


# Add a document to my-index
POST /my-index/_doc
{
  "curatedTags": {
    "added": "foo",
    "name": "et"
  }
}

POST /my-index/_doc
{
  "curatedTags": {
    "added": "foo",
    "name": "no"
  }
}

GET /my-index/_search
{
  "query": {
    "terms": {
      "curatedTags.added": [
        {
          "added": "foo",
          "name": "et"
        },
        {
          "added": "foo",
          "name": "dolores"
        }
      ]
    }
  }
}

Response:

{
  "took": 7,
  "timed_out": false,
  "_shards": {
    "total": 1,
    "successful": 1,
    "skipped": 0,
    "failed": 0
  },
  "hits": {
    "total": {
      "value": 2,
      "relation": "eq"
    },
    "max_score": 1,
    "hits": [
      {
        "_index": "my-index",
        "_id": "IMbdy5QBS5uc1xLClKFI",
        "_score": 1,
        "_source": {
          "curatedTags": {
            "added": "foo",
            "name": "et"
          }
        }
      },
      {
        "_index": "my-index",
        "_id": "Icbdy5QBS5uc1xLCvqFt",
        "_score": 1,
        "_source": {
          "curatedTags": {
            "added": "foo",
            "name": "no"
          }
        }
      }
    ]
  }
}

@github-actions
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Following you can find the validation results for the APIs you have changed.

API Status Request Response
esql.async_query Missing test Missing test
esql.query 284/284 Missing test

You can validate these APIs yourself by using the make validate target.

@github-actions
Copy link
Contributor

Following you can find the validation results for the APIs you have changed.

API Status Request Response
esql.async_query Missing test Missing test
esql.query 287/287 Missing test

You can validate these APIs yourself by using the make validate target.

@github-actions
Copy link
Contributor

Following you can find the validation results for the API you have changed.

API Status Request Response
search 🔴 2284/2367 2350/2351

You can validate this API yourself by using the make validate target.

@flobernd
Copy link
Member Author

Info: We decided to keep the ES|QL definition as is, even though this will prevent named parameters from being used. The inherent problem with the ES|QL parameters definition will be fixed in a separate PR (which will cause a breaking change).

@flobernd flobernd merged commit 97160e1 into main Feb 10, 2025
8 checks passed
@flobernd flobernd deleted the fieldvalue-scalar branch February 10, 2025 09:41
@github-actions
Copy link
Contributor

The backport to 8.x failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.x 8.x
# Navigate to the new working tree
cd .worktrees/backport-8.x
# Create a new branch
git switch --create backport-3687-to-8.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 97160e198d14bed12e704be8eb34204828dcbc97
# Push it to GitHub
git push --set-upstream origin backport-3687-to-8.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.x

Then, create a pull request where the base branch is 8.x and the compare/head branch is backport-3687-to-8.x.

@github-actions
Copy link
Contributor

The backport to 8.18 failed:

The process '/usr/bin/git' failed with exit code 1

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-8.18 8.18
# Navigate to the new working tree
cd .worktrees/backport-8.18
# Create a new branch
git switch --create backport-3687-to-8.18
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 97160e198d14bed12e704be8eb34204828dcbc97
# Push it to GitHub
git push --set-upstream origin backport-3687-to-8.18
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-8.18

Then, create a pull request where the base branch is 8.18 and the compare/head branch is backport-3687-to-8.18.

github-actions bot pushed a commit that referenced this pull request Feb 10, 2025
pquentin pushed a commit that referenced this pull request Feb 13, 2025
(cherry picked from commit 97160e1)

Co-authored-by: Florian Bernd <git@flobernd.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants