-
-
Notifications
You must be signed in to change notification settings - Fork 540
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
[kvexec] merge join #8561
[kvexec] merge join #8561
Conversation
#benchmark |
@max-hoffman DOLT
|
@coffeegoddd DOLT
|
@max-hoffman DOLT
|
@max-hoffman DOLT
|
@max-hoffman DOLT
|
@coffeegoddd DOLT
|
#benchmark |
@max-hoffman DOLT
|
@max-hoffman DOLT
|
@max-hoffman DOLT
|
@max-hoffman DOLT
|
@max-hoffman DOLT
|
@max-hoffman DOLT
|
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.
I think using opposite logic is more readable:
https://github.com/dolthub/dolt/compare/max/kv-merge-join...james/refactor?expand=1
While goto
s aren't the best, I think it's still pretty understandable, so LGTM
@max-hoffman DOLT
|
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.
Not as bad as you built it up to be, generally not too hard to understand.
I think readability would be improved by making the loops explicit, keeping goto statements for true jumps rather than "go to beginning of this loop"
@max-hoffman DOLT
|
@coffeegoddd DOLT
|
@max-hoffman DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
@coffeegoddd DOLT
|
This isn't the best perf win on linux, but it counteracts the
sql.Row
interface PR which otherwise would swing merge join +30% in the wrong direction.TODO: