Skip to content

Editing analyze rule and adding CI - #5710

Merged
ahkcs merged 4 commits into
opensearch-project:mainfrom
Krish-Gandhi:feature/analyze-expensive-sort-topk
Aug 20, 2026
Merged

Editing analyze rule and adding CI#5710
ahkcs merged 4 commits into
opensearch-project:mainfrom
Krish-Gandhi:feature/analyze-expensive-sort-topk

Conversation

@Krish-Gandhi

@Krish-Gandhi Krish-Gandhi commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Description

  • Updating Expensive Sort analyze rule to account for CalciteEnumerableTopK case
  • Adding IT as well
  • Adding rules to endpoint.md

Currently, a head + sort command becomes a CalciteEnumerableTopK, so this rule will miss sort operations if the query also contains head (which it will with fetch_size). Added "topk" in rule check to fix.

Related Issues

Related to #5658
Related to #5568
Related to #5500

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.

Signed-off-by: Krish Gandhi <kjg2352@gmail.com>
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 86a3cd4)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

@Krish-Gandhi
Krish-Gandhi marked this pull request as ready for review August 20, 2026 19:01
@ahkcs ahkcs added the maintenance Improves code quality, but not the product label Aug 20, 2026
Signed-off-by: Krish Gandhi <kjg2352@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 5f0fa31

Signed-off-by: Krish Gandhi <kjg2352@gmail.com>
@github-actions

github-actions Bot commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 86a3cd4
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Simplify conditional logic flow

The condition uses AND logic, which means a node must contain NEITHER "sort" NOR
"topk" to be skipped. This is correct, but consider using OR with negation for
clarity: if (name.contains("sort") || name.contains("topk")) with inverted logic
flow.

core/src/main/java/org/opensearch/sql/executor/analyze/ExpensiveSortRule.java [35-37]

 String name = node.getNode().toLowerCase(Locale.ROOT);
-if (!name.contains("sort") && !name.contains("topk")) {
+if (name.contains("sort") || name.contains("topk")) {
+  // Process expensive sort logic
+} else {
   continue;
 }
Suggestion importance[1-10]: 3

__

Why: The suggestion proposes inverting the logic from !name.contains("sort") && !name.contains("topk") to name.contains("sort") || name.contains("topk") with an else block. While both are logically equivalent and the suggestion is technically correct, this is a minor stylistic preference that doesn't significantly improve readability or functionality. The current implementation using De Morgan's law with early continue is a common pattern and equally clear.

Low

Previous suggestions

Suggestions up to commit 5f0fa31
CategorySuggestion                                                                                                                                    Impact
General
Simplify boolean logic for clarity

The condition uses AND logic which means both "sort" and "topk" must be absent to
skip the node. This is correct, but consider extracting the node name conversion to
avoid repeated toLowerCase calls if this code is in a hot path. Additionally, verify
that all TopK variants are covered by the "topk" substring check.

core/src/main/java/org/opensearch/sql/executor/analyze/ExpensiveSortRule.java [35-37]

 String name = node.getNode().toLowerCase(Locale.ROOT);
-if (!name.contains("sort") && !name.contains("topk")) {
+if (!(name.contains("sort") || name.contains("topk"))) {
   continue;
 }
Suggestion importance[1-10]: 4

__

Why: The suggestion correctly identifies that !(name.contains("sort") || name.contains("topk")) is logically equivalent to !name.contains("sort") && !name.contains("topk") by De Morgan's law. While the improved version may be slightly more readable to some developers, both forms are equally correct and the improvement is marginal. The existing code is clear enough and the change doesn't significantly impact functionality or performance.

Low

Signed-off-by: Krish Gandhi <kjg2352@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 86a3cd4

1 similar comment
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 86a3cd4

@ahkcs
ahkcs merged commit dcdad53 into opensearch-project:main Aug 20, 2026
39 of 41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

maintenance Improves code quality, but not the product

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants