-
Notifications
You must be signed in to change notification settings - Fork 21
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
Add support for left and right joins #23
Conversation
ndjson-join
Outdated
@@ -13,6 +13,8 @@ commander | |||
.usage("[options] [expression₀ [expression₁]] <file₀> <file₁>") | |||
.description("Join two newline-delimited JSON streams into a stream of pairs.") | |||
.option("-r, --require <name=module>", "require a module", requires, {d: undefined, i: -1}) | |||
.option("--left", "Perform a left join", false) | |||
.option("--right", "Perform a right join", false) |
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.
Lowercase “perform”.
ndjson-join
Outdated
@@ -25,6 +27,8 @@ if (commander.args.length < 4) { | |||
commander.args.splice(1, 0, commander.args[0]); | |||
} | |||
|
|||
var join = commander.left ? leftJoin : commander.right ? rightJoin : innerJoin; | |||
|
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.
So if both --left
and --right
are set, --left
takes priority? Perhaps instead, if both are set we should do an outerJoin?
ndjson-join
Outdated
map.forEach(function(value, key) { | ||
value[0].forEach(function(v0) { | ||
if (value[1].length === 0) { | ||
output([v0, null]) |
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.
Missing semicolon.
ndjson-join
Outdated
function leftJoin() { | ||
map.forEach(function(value, key) { | ||
value[0].forEach(function(v0) { | ||
if (value[1].length === 0) { |
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’d invert the logic like so:
if (value[1].length) {
…
} else {
output([v0, null]);
}
ndjson-join
Outdated
map.forEach(function(value, key) { | ||
value[1].forEach(function(v1) { | ||
if (value[0].length === 0) { | ||
output([null, v1]) |
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.
Semicolon.
ndjson-join
Outdated
function rightJoin() { | ||
map.forEach(function(value, key) { | ||
value[1].forEach(function(v1) { | ||
if (value[0].length === 0) { |
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.
Invert logic as above.
Good stuff! Biggest suggestion would be to support outer join either as Can you update the README to document the new arguments? |
Done! Thanks for the feedback. One thing I wondered: wouldn't it be better to output an empty object instead of |
Released in 0.3.1! Thank you! |
Fixes #21.
I also added a new test file. The results are the following: