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

Allow zero-row files, and handle stream exceptions #106

Merged
merged 2 commits into from
Dec 29, 2020

Conversation

gregplaysguitar
Copy link
Contributor

@gregplaysguitar gregplaysguitar commented Jun 16, 2020

Creating zero-row files is uncommon, but sometimes useful for handling edge cases in a pipeline, so there's no need to arbitrarily prevent it.

The ParquetTransformer change allows exceptions to be caught and dealt with upstream when dealing with streams, previously they were being swallowed.

This change allows exceptions to be caught and dealt with upstream when dealing with streams
@asmuth
Copy link
Contributor

asmuth commented Jun 16, 2020

The first change (allow zero-row files) looks good to me.

Can't say much about the second change, since I'm not an expert on JavaScript streams, but I took a quick look at the nodejs docs. Based on that, the proposed change looks correct to me and what we currently have seems to be incorrect. Maybe @kessler can also comment on the streams issue.

@annfomenko
Copy link

Hello @asmuth! This PR looks exactly that I need [https://github.com//issues/116]. When do you plan to merge it?

} else {
callback();
callback()
Copy link
Contributor

Choose a reason for hiding this comment

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

This repo seems to be using semicolons, might want to go along with that.

@@ -264,14 +260,14 @@ class ParquetTransformer extends stream.Transform {

_transform(row, encoding, callback) {
if (row) {
this.writer.appendRow(row).then(callback);
this.writer.appendRow(row).then(d => callback(null, d), callback)
Copy link
Contributor

@dobesv dobesv Dec 28, 2020

Choose a reason for hiding this comment

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

since d is always undefined, you can do:

Suggested change
this.writer.appendRow(row).then(d => callback(null, d), callback)
this.writer.appendRow(row).then(() => callback(), callback)

}
}

_flush(callback) {
this.writer.close(callback);
this.writer.close(callback).then(d => callback(null, d), callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
this.writer.close(callback).then(d => callback(null, d), callback)
this.writer.close(callback).then(() => callback(), callback)

@dobesv
Copy link
Contributor

dobesv commented Dec 28, 2020

PR looks good in general. Unfortunately I cannot merge it and the maintainers of this project don't seem to be merging anything for more than a year (or two?)

@kessler
Copy link
Member

kessler commented Dec 29, 2020

@dobesv very sorry my friend. I am well aware that this project has been neglected and I really really hope I get to do something about it some time. I went over the PR and more importantly Paul did, so it's merged and I'll publish to npm asap.
My sincere apologies again.

@kessler kessler merged commit d351f3b into ironSource:master Dec 29, 2020
@dobesv
Copy link
Contributor

dobesv commented Dec 29, 2020

@kessler It's OK, it's the nature of volunteer work - sometimes we just have better things to do!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants