Skip to content

Conversation

@Swiddis
Copy link
Collaborator

@Swiddis Swiddis commented Dec 3, 2025

Description

Seems to delete a major perf bottleneck: we create an access controller context for every individual document we process in a script, which involves a lot of stack trace traversal and locking for accessing that context. Curious what the breakage is if I just delete this controller step, since it passes tests locally. At least for the query I was interested in, it reduced the runtime from 70 seconds (2mil documents) to 4.

Better approach if we need it: move the privilege step to outside the core script loop.

Measured from slow query (big5 dataset):

source = big5
| where `agent.name` = 'filebeat'
| where `@timestamp` >= '2023-01-01 00:00:00' and `@timestamp` <= '2023-01-02 00:00:00'
| where `metrics.size` > 1000 and `metrics.size` != 5000
| stats count(`agent.name`)

Before (70s/query):
image

After (4s/query):
image

Related Issues

N/A

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 3, 2025

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@Swiddis Swiddis added performance Make it fast! calcite calcite migration releated labels Dec 3, 2025
@Swiddis Swiddis changed the title Remove access controller step in Calcite script Remove access controller step in Calcite script execution Dec 3, 2025
@Swiddis Swiddis changed the title Remove access controller step in Calcite script execution Remove doPrivileged call in Calcite script execution Dec 3, 2025
@Swiddis Swiddis added the maintenance Improves code quality, but not the product label Dec 3, 2025
@songkant-aws
Copy link
Contributor

Grreat! I remember somehow 2.19 still needs it. But it was removed in OpenSearch since 3.x version.

@Swiddis Swiddis requested a review from RyanL1997 as a code owner December 4, 2025 16:47
@penghuo penghuo enabled auto-merge (squash) December 4, 2025 17:19
Copy link
Collaborator

@dai-chen dai-chen 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 the changes! If I recall right, this is mostly for action whitelisted in plugin-security.policy?

@penghuo penghuo merged commit 2942d4b into opensearch-project:main Dec 4, 2025
40 of 53 checks passed
@Swiddis Swiddis mentioned this pull request Dec 9, 2025
8 tasks
asifabashar pushed a commit to asifabashar/sql that referenced this pull request Dec 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

calcite calcite migration releated maintenance Improves code quality, but not the product performance Make it fast!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants