-
Notifications
You must be signed in to change notification settings - Fork 25.4k
[Streams] Propagate the ingest pipeline access pattern flag to the ingest document #130488
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
[Streams] Propagate the ingest pipeline access pattern flag to the ingest document #130488
Conversation
…ent when executing
Pinging @elastic/es-data-management (Team:Data Management) |
/** | ||
* @return The access pattern for any currently executing pipelines, or null if no pipelines are in progress for this doc | ||
*/ | ||
public IngestPipelineFieldAccessPattern getCurrentAccessPattern() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it intentional that this method isn't used above (and seems to only be used in the test)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not used above yet. I'm currently using it in a couple places in the next set of changes I'm working on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces tracking of each pipeline’s access pattern within nested pipeline executions by maintaining a stack on the ingest document. Key changes include:
- Add
accessPatternStack
field and related push/pop logic inIngestDocument
- Update
Pipeline
class to allow mocking by removingfinal
- Introduce a comprehensive nested-access-pattern test in
IngestDocumentTests
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
server/src/main/java/org/elasticsearch/ingest/IngestDocument.java | Added accessPatternStack , push/pop logic, and getCurrentAccessPattern() |
server/src/main/java/org/elasticsearch/ingest/Pipeline.java | Removed final modifier from Pipeline for testability |
server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java | Added testNestedAccessPatternPropagation with Mockito captures |
Comments suppressed due to low confidence (2)
server/src/main/java/org/elasticsearch/ingest/Pipeline.java:28
- [nitpick] Removing
final
fromPipeline
makes it extendable, which may not be intended for core pipeline behavior; verify that this aligns with the project's API design.
public class Pipeline {
server/src/test/java/org/elasticsearch/ingest/IngestDocumentTests.java:1273
- The test uses
is(empty())
butis
is not statically imported; addimport static org.hamcrest.Matchers.is;
to avoid a compilation error.
assertThat(document.getPipelineStack(), is(empty()));
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor niggles from CoPilot but otherwise LGTM 👍🏻
Adds the ability for an ingest document to capture the pipeline's access pattern flag when executing the pipeline on the document. The document stores the access pattern flag in a stack to allow for holding and restoring the access pattern flag when running nested pipelines.