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.

Sign up for GitHub

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

Jump to bottom

Unify query planner and executor #307

Closed
wants to merge 58 commits into from
Closed

Unify query planner and executor #307

wants to merge 58 commits into from

Conversation

zesiar0
Copy link
Contributor

@zesiar0 zesiar0 commented Jul 27, 2023

related issue apache/skywalking#10562.

@hanahmily hanahmily added this to the 0.5.0 milestone Jul 31, 2023
@hanahmily hanahmily added the enhancement New feature or request label Jul 31, 2023
@lujiajing1126
Copy link
Contributor

Pls fix CI first @zesiar0

hanahmily
hanahmily requested changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@hanahmily hanahmily left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, the apache/skywalking#10562 tends to merge TopN and Measure's query process. (@lujiajing1126 feel free to correct me)

However, the current implementation introduces a new analyzer. Although the new topn analyzer takes inspiration from the measure analyzer, it is not heading in the right direction.

@hanahmily
Copy link
Contributor

@zesiar0 Could you fix the test cases? The current structure has almost reached the design goals.

@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2023 •
edited
Loading

Codecov Report

Merging #307 (cca5bcf) into main (63be6e8) will decrease coverage by 1.04%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main     #307      +/-   ##
==========================================
- Coverage   39.83%   38.79%   -1.04%     
==========================================
  Files         109      109              
  Lines       11883    12199     +316     
==========================================
  Hits         4733     4733              
- Misses       6683     6999     +316     
  Partials      467      467
Files Changed Coverage Δ
pkg/query/logical/measure/measure_analyzer.go 0.00% <0.00%> (ø)
.../query/logical/measure/measure_plan_aggregation.go 0.00% <0.00%> (ø)
pkg/query/logical/measure/measure_plan_groupby.go 0.00% <0.00%> (ø)
...ry/logical/measure/measure_plan_indexscan_local.go 0.00% <0.00%> (ø)
pkg/query/logical/measure/measure_plan_top.go 0.00% <0.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@zesiar0
Copy link
Contributor Author

zesiar0 commented Sep 19, 2023 •
edited
Loading

Comparing 5801363 and f70e3ce, I just modify source code related to data query and method comments. And Test (Asia/Shanghai) job succeeds in commit 5801363 but failed in f70e3ce because of data race. So is it possible there is a bug? @hanahmily @lujiajing1126

@hanahmily hanahmily modified the milestones: 0.5.0, 0.6.0 Oct 17, 2023
@zesiar0 zesiar0 closed this Oct 19, 2023
@zesiar0 zesiar0 mentioned this pull request Oct 31, 2023
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanahmily hanahmily hanahmily requested changes

@lujiajing1126 lujiajing1126 Awaiting requested review from lujiajing1126

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
0.6.0
Development

Successfully merging this pull request may close these issues.
5 participants
@zesiar0 @lujiajing1126 @hanahmily @codecov-commenter @wu-sheng