Skip to content

Conversation

@eyalkoren
Copy link
Contributor

@eyalkoren eyalkoren commented Feb 6, 2025

This PR separates the Field APIs into write and read and exposes only the read APIs for use within ingest conditional scripts.
Things to note:

  • Immutability: all read APIs, including iterators, must not be able to mutate the source map
  • Performance: since the conditional script factory now needs to create script instances with the ctx map, we cannot reuse a single script instance; instead, we precompile the factory and create a script instance in each invocation

@eyalkoren
Copy link
Contributor Author

@elasticmachine test this please

Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

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

This is a good start! I left some feedback for a few items that need to be looked at.

@eyalkoren eyalkoren marked this pull request as ready for review June 26, 2025 10:31
@eyalkoren eyalkoren requested a review from a team as a code owner June 26, 2025 10:31
@eyalkoren eyalkoren requested a review from jdconrad June 26, 2025 10:31
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Jun 26, 2025
@eyalkoren eyalkoren added >feature :Core/Infra/Core Core issues without another label and removed needs:triage Requires assignment of a team area label labels Jun 26, 2025
@eyalkoren eyalkoren force-pushed the support-dotted-fields-in-ingest-processors branch from 50c278c to 5a60385 Compare July 8, 2025 18:55
@eyalkoren eyalkoren requested a review from jdconrad July 8, 2025 18:58
@eyalkoren
Copy link
Contributor Author

@jdconrad I had a bit of a mess and had to force push, please review carefully. I went over it and it looks fine, but since it is a small one, better go over everything in details.

@eyalkoren
Copy link
Contributor Author

The CI failures seem somewhat arbitrary lately, making it hard to tell what needs further investigation and what simply needs another build attempt.

@jdconrad your assistance in this regard would be appreciated. The consistent failures are around the Serverless checks and some bwc tests. If you have any pointers on where to look or what to fix - that'd be great.

@ldematte
Copy link
Contributor

Tests in BWC look unrelated -- there are a lot of ESQL tests failing there. A rebase should help.
The serverless failure instead seems to be related -- the test expects a ScriptException but gets a UnsupportedOperationException. @jdconrad we might need your input here -- seems the exception is failing to get wrapped?

@jdconrad
Copy link
Contributor

I will take another pass through this today.

Copy link
Contributor

@jdconrad jdconrad left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the iterations.

@jdconrad
Copy link
Contributor

I've reproduced the exception in serverless locally, and what I have found is it's a serialization issue. If the script exception is not serialized then it gets sent directly back as x-content. However, if it is serialized, it gets wrapped in not serializable exception wrapper which is then sent back as a proxy for the script exception in x-content. This is happening because the unsupported operation exception isn't serializable which is thrown when the unmodifiable collection is written to.

In serverless it is being serialized and in non-serverless is isn't. I assume this is due to the way the clusters for the tests are configued, but I haven't looked too closely into this.

For now I would suggest that we check to make sure an exception comes back with the catch_bad_request parameter, but not check what the type of exception is. In the future, we may want to have Painless handle this specific exception in a different way or have elasticsearch exception add unsupported operation exception to its custom serialization list.

@eyalkoren
Copy link
Contributor Author

For sure this inconsistency should not hold this PR from being merged, so I will adjust my assertion.
However, I am not sure we should just forget about it...

I assume this is due to the way the clusters for the tests are configued, but I haven't looked too closely into this.

If it's a matter of test clusters setup, fixing it is nice-to-have (really nice 🙂 ).
If it's a systematic inconsistency between serverless and non-serverless - I think this should be considered a bug. For example, someone could specifically catch a ScriptException and base a behavior on that, thus worsening inconsistency.

@jdconrad
Copy link
Contributor

This can potentially happen in just normal elasticsearch as well, but would require the requests to be serialized as part of the test cases. Either way this change still LGTM!

@eyalkoren eyalkoren merged commit a6f0f6f into elastic:main Jul 16, 2025
33 checks passed
@eyalkoren eyalkoren deleted the support-dotted-fields-in-ingest-processors branch July 16, 2025 08:46
szybia added a commit to szybia/elasticsearch that referenced this pull request Jul 16, 2025
…king

* upstream/main: (91 commits)
  Mute org.elasticsearch.packaging.test.DockerTests test130JavaHasCorrectOwnership elastic#131369
  Add exception logging when interrupted (elastic#131153)
  Mute org.elasticsearch.packaging.test.DockerTests test140CgroupOsStatsAreAvailable elastic#131372
  Mute org.elasticsearch.packaging.test.DockerTests test070BindMountCustomPathConfAndJvmOptions elastic#131366
  Mute org.elasticsearch.xpack.test.rest.XPackRestIT test {p0=ml/delete_expired_data/Test delete expired data with body parameters} elastic#131364
  Mute org.elasticsearch.xpack.esql.vector.VectorSimilarityFunctionsIT testSimilarityBetweenConstantVectorAndField {functionName=v_cosine similarityFunction=COSINE} elastic#131363
  Mute org.elasticsearch.xpack.esql.vector.VectorSimilarityFunctionsIT testDifferentDimensions {functionName=v_cosine similarityFunction=COSINE} elastic#131362
  Mute org.elasticsearch.xpack.esql.vector.VectorSimilarityFunctionsIT testSimilarityBetweenConstantVectors {functionName=v_cosine similarityFunction=COSINE} elastic#131361
  Check SCORE_FUNCTION capability in VerifierTests (elastic#131352)
  Replace deprecated routingTable table call in tests (elastic#131005)
  [DOCS] Remove misused applies_to tag (elastic#131349)
  Adj ivf postings list building (elastic#130843)
  [Transform] Read metadata from Project State (elastic#131205)
  Add note on o11y to architecture guide (elastic#131291)
  Upgrade AWS Java SDK to 2.31.78 (elastic#131050)
  Support Fields API in conditional ingest processors (elastic#121914)
  ESQL - KNN function uses prefilters when pushed down to Lucene (elastic#131004)
  Add docs for ES|QL query logs (elastic#131287)
  Simplify `expectedFinalRegisterValue` computation (elastic#131274)
  Mute org.elasticsearch.test.rest.yaml.RcsCcsCommonYamlTestSuiteIT test {p0=search/110_field_collapsing/field collapsing, inner_hits and maxConcurrentGroupRequests} elastic#131348
  ...
@eyalkoren eyalkoren restored the support-dotted-fields-in-ingest-processors branch July 17, 2025 14:21
eyalkoren added a commit that referenced this pull request Jul 17, 2025
jdconrad pushed a commit that referenced this pull request Jul 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

:Core/Infra/Core Core issues without another label >feature Team:Core/Infra Meta label for core/infra team v9.2.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants