[Swiddis]

Description

In cases with over 10k nested fields, the nested loop in this function is very slow. Rewrite to O(n).

In a local benchmarking case with ~70000 fields, this reduced query times from 40s to 600ms.

Profile data from before state (frame shows up across other threads, in total takes up ~97.5%):

--- Execution profile ---
Total samples       : 18885
skipped             : 95 (0.50%)

--- 37560000000 ns (19.67%), 3755 samples
  [ 0] java.util.AbstractList$Itr.hasNext
  [ 1] java.util.AbstractList.indexOf
  [ 2] org.apache.calcite.tools.RelBuilder.field
  [ 3] org.apache.calcite.tools.RelBuilder.field
  [ 4] org.opensearch.sql.calcite.CalciteRelNodeVisitor.lambda$tryToRemoveNestedFields$10
  [ 5] org.opensearch.sql.calcite.CalciteRelNodeVisitor$$Lambda.0x00007fb475274ee0.apply
  [ 6] java.util.stream.ReferencePipeline$3$1.accept
  [ 7] java.util.stream.ReferencePipeline$2$1.accept
  [ 8] java.util.HashMap$KeySpliterator.forEachRemaining
  [ 9] java.util.stream.AbstractPipeline.copyInto
  [10] java.util.stream.AbstractPipeline.wrapAndCopyInto
  [11] java.util.stream.AbstractPipeline.evaluate
  [12] java.util.stream.AbstractPipeline.evaluateToArrayNode
  [13] java.util.stream.ReferencePipeline.toArray
  [14] java.util.stream.ReferencePipeline.toArray
  [15] java.util.stream.ReferencePipeline.toList
  [16] org.opensearch.sql.calcite.CalciteRelNodeVisitor.tryToRemoveNestedFields
  [17] org.opensearch.sql.calcite.CalciteRelNodeVisitor.handleAllFieldsProject
  [18] org.opensearch.sql.calcite.CalciteRelNodeVisitor.visitProject
  [19] org.opensearch.sql.calcite.CalciteRelNodeVisitor.visitProject
  [20] org.opensearch.sql.ast.tree.Project.accept
  [21] org.opensearch.sql.calcite.CalciteRelNodeVisitor.analyze
  [22] org.opensearch.sql.executor.QueryService.analyze
  [23] org.opensearch.sql.executor.QueryService.lambda$executeWithCalcite$0
  [24] org.opensearch.sql.executor.QueryService$$Lambda.0x00007fb475339c28.get
  [25] org.opensearch.sql.common.error.StageErrorHandler.executeStage
  [26] org.opensearch.sql.executor.QueryService.lambda$executeWithCalcite$2
  [27] org.opensearch.sql.executor.QueryService$$Lambda.0x00007fb4753395c0.run
  [28] org.opensearch.sql.calcite.utils.CalciteClassLoaderHelper.lambda$withCalciteClassLoader$0
  [29] org.opensearch.sql.calcite.utils.CalciteClassLoaderHelper$$Lambda.0x00007fb4750a3590.call
  [30] org.opensearch.sql.calcite.utils.CalciteClassLoaderHelper.withCalciteClassLoader
  [31] org.opensearch.sql.calcite.utils.CalciteClassLoaderHelper.withCalciteClassLoader
  [32] org.opensearch.sql.executor.QueryService.lambda$executeWithCalcite$3
  [33] org.opensearch.sql.executor.QueryService$$Lambda.0x00007fb475339398.run
  [34] org.opensearch.sql.calcite.CalcitePlanContext.run
  [35] org.opensearch.sql.executor.QueryService.executeWithCalcite
  [36] org.opensearch.sql.executor.QueryService.execute
  [37] org.opensearch.sql.executor.execution.QueryPlan.execute
  [38] org.opensearch.sql.opensearch.executor.OpenSearchQueryManager$$Lambda.0x00007fb4750a15b8.run
  [39] org.opensearch.sql.opensearch.executor.OpenSearchQueryManager.lambda$schedule$1
  [40] org.opensearch.sql.opensearch.executor.OpenSearchQueryManager$$Lambda.0x00007fb4750a17e0.run
  [41] org.opensearch.sql.opensearch.executor.OpenSearchQueryManager.lambda$withCurrentContext$2
  [42] org.opensearch.sql.opensearch.executor.OpenSearchQueryManager$$Lambda.0x00007fb4750a1a08.run
  [43] org.opensearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run
  [44] java.util.concurrent.ThreadPoolExecutor.runWorker
  [45] java.util.concurrent.ThreadPoolExecutor$Worker.run
  [46] java.lang.Thread.runWith
  [47] java.lang.Thread.run

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.

[github-actions]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests

🔒 No security concerns identified

✅ No TODO sections

🔀 No multiple PR themes

⚡ Recommended focus areas for review Possible Issue The logic assumes that a field with a dot is nested only if its parent prefix exists in allFields. However, if field is "a.b.c" and both "a" and "a.b" exist in allFields, only "a" is checked (the immediate parent of the last segment). This means "a.b.c" would be kept even though "a.b" is its direct parent. The old code filtered any field where any parent prefix existed. This change may incorrectly retain deeply nested fields when an intermediate parent is present. String field = fieldNames.get(i); int lastDot = field.lastIndexOf('.'); if (lastDot == -1 || !allFields.contains(field.substring(0, lastDot))) { keptFields.add(context.relBuilder.field(i)); }

[github-actions]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Handle edge case for dot position The substring operation field.substring(0, lastDot) can throw StringIndexOutOfBoundsException if lastDot is 0 (field starts with a dot). Add a check to ensure lastDot > 0 before performing the substring operation to prevent potential crashes. core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java [674-681] List<RexNode> keptFields = new ArrayList<>(fieldNames.size()); for (int i = 0; i < fieldNames.size(); i++) { String field = fieldNames.get(i); int lastDot = field.lastIndexOf('.'); - if (lastDot == -1 || !allFields.contains(field.substring(0, lastDot))) { + if (lastDot <= 0 || !allFields.contains(field.substring(0, lastDot))) { keptFields.add(context.relBuilder.field(i)); } } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that field.substring(0, lastDot) would return an empty string when lastDot is 0, which is a valid edge case. Changing the condition from lastDot == -1 to lastDot <= 0 prevents checking for an empty parent field name in allFields, improving robustness.	Medium

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.38%. Comparing base (24bfaaf) to head (f094f58).

❌ Your project check has failed because the head coverage (75.38%) is below the target coverage (99.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff            @@
##               main    #5913   +/-   ##
=========================================
  Coverage     75.38%   75.38%           
+ Complexity    10386    10383    -3     
=========================================
  Files           962      962           
  Lines         41089    41079   -10     
  Branches       4681     4679    -2     
=========================================
- Hits          30975    30969    -6     
+ Misses         8806     8804    -2     
+ Partials       1308     1306    -2     

Flag	Coverage Δ
sql-engine	75.38% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.