-
Notifications
You must be signed in to change notification settings - Fork 92
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
Feat sort merge join #668
Feat sort merge join #668
Conversation
Codecov ReportPatch coverage:
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. Additional details and impacted files@@ Coverage Diff @@
## master #668 +/- ##
==========================================
+ Coverage 36.77% 37.29% +0.52%
==========================================
Files 229 230 +1
Lines 24016 24429 +413
==========================================
+ Hits 8832 9112 +280
- Misses 14177 14273 +96
- Partials 1007 1044 +37
☔ View full report in Codecov by Sentry. |
|
||
var outerValue, innerValue proto.Value | ||
|
||
for { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- should 'yield' current compare checkpoint rows, consider: [1,2,3] inner join [2,2,2,3,3,3].
- should handle all join types, not only inner join.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
Please add unit tests. |
|
||
func (s *SortMergeJoin) Next() (proto.Row, error) { | ||
// all data is order | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please remove the unnecessary blank line at the function beginning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
pkg/dataset/sort_merge_join.go
Outdated
func (s *SortMergeJoin) resGenerate(leftRow proto.Row, rightRow proto.Row) proto.Row { | ||
res := make([]proto.Value, leftRow.Length()+rightRow.Length()) | ||
leftValue := make([]proto.Value, leftRow.Length()) | ||
rightValue := make([]proto.Value, rightRow.Length()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if rightRow is nil(refer to line 100), would this cause NPE?
pkg/dataset/sort_merge_join.go
Outdated
} | ||
|
||
func (s *SortMergeJoin) Next() (proto.Row, error) { | ||
// all data is order |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it's better to move the comment into the SortMergeJoin struct like this:
//SortMergeJoin ensures that the data sets participating in the join are in order before using sort-merge join.
type SortMergeJoin struct {
fields []proto.Field
joinColumn *JoinColumn
joinType ast.JoinType
outer proto.Dataset
inner proto.Dataset
}
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
pkg/dataset/sort_merge_join.go
Outdated
package dataset | ||
|
||
import ( | ||
"go.uber.org/atomic" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
挪到第二个 import 里面
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wang1309 at root package, use this tool:
go get -u github.com/dubbogo/tools/cmd/imports-formatter
imports-formatter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
SonarCloud Quality Gate failed. 0 Bugs No Coverage information Catch issues before they fail your Quality Gate with our IDE extension SonarLint |
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
* add sort merge join file * feat:add sort merge join * feat: sort merge join feature add * temp commit * add unit test and optimize code * delete unuse file * fixed: fix review dog fault * add comment * format import --------- Co-authored-by: wangrui130 <[email protected]>
What this PR does:
sort merge join dataset
Which issue(s) this PR fixes:
Fixes #642
Special notes for your reviewer:
Does this PR introduce a user-facing change?: