Skip to content
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

Fix result order of parse with other run time fields #934

Merged
merged 2 commits into from
Oct 22, 2022

Conversation

joshuali925
Copy link
Member

Signed-off-by: Joshua Li joshuali925@gmail.com

Description

ProjectOperator assumes derived fields comes at last, since they are generated at run time. it's incorrect since there could be other run time fields (e.g. eval, AD). This PR fixes it.

Maybe parse should not allow overwriting original fields so ProjectOperator doesn't have to maintain a separate named expression list and handle conflicts.

Issues Resolved

[List any issues this PR will resolve]

Check List

  • New functionality includes testing.
    • All tests pass, including unit test, integration test and doctest
  • New functionality has been documented.
    • New functionality has javadoc added
    • New functionality has user manual doc added
  • Commits are signed per the DCO using --signoff

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: Joshua Li <joshuali925@gmail.com>
@joshuali925 joshuali925 requested a review from a team as a code owner October 19, 2022 20:03
@joshuali925 joshuali925 self-assigned this Oct 19, 2022
@joshuali925 joshuali925 added the bug Something isn't working label Oct 19, 2022
@codecov-commenter
Copy link

codecov-commenter commented Oct 19, 2022

Codecov Report

Merging #934 (1fca816) into 2.x (057fa44) will decrease coverage by 2.77%.
The diff coverage is 99.50%.

❗ Current head 1fca816 differs from pull request most recent head 927230a. Consider uploading reports for the commit 927230a to get more accurate results

@@             Coverage Diff              @@
##                2.x     #934      +/-   ##
============================================
- Coverage     97.87%   95.10%   -2.78%     
- Complexity     3020     3069      +49     
============================================
  Files           284      303      +19     
  Lines          7425     8246     +821     
  Branches        475      609     +134     
============================================
+ Hits           7267     7842     +575     
- Misses          157      350     +193     
- Partials          1       54      +53     
Flag Coverage Δ
query-workbench 62.76% <ø> (?)
sql-engine 97.90% <99.50%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
...rg/opensearch/sql/analysis/ExpressionAnalyzer.java 100.00% <ø> (ø)
...h/sql/expression/function/OpenSearchFunctions.java 100.00% <ø> (ø)
...theus/data/constants/PrometheusFieldConstants.java 0.00% <0.00%> (ø)
...ain/java/org/opensearch/sql/analysis/Analyzer.java 100.00% <100.00%> (ø)
...org/opensearch/sql/analysis/HighlightAnalyzer.java 100.00% <100.00%> (ø)
...org/opensearch/sql/analysis/model/CatalogName.java 100.00% <100.00%> (ø)
...ql/analysis/model/CatalogSchemaIdentifierName.java 100.00% <100.00%> (ø)
.../org/opensearch/sql/analysis/model/SchemaName.java 100.00% <100.00%> (ø)
...opensearch/sql/expression/HighlightExpression.java 100.00% <100.00%> (ø)
...ensearch/sql/planner/logical/LogicalHighlight.java 100.00% <100.00%> (ø)
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@Yury-Fridlyand
Copy link
Collaborator

Can you make a unit test which covers this case?

Signed-off-by: Joshua Li <joshuali925@gmail.com>
@joshuali925 joshuali925 added the v2.4.0 'Issues and PRs related to version v2.4.0' label Oct 21, 2022
@joshuali925 joshuali925 merged commit 30a2d27 into opensearch-project:2.x Oct 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working v2.4.0 'Issues and PRs related to version v2.4.0'
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants