-
Notifications
You must be signed in to change notification settings - Fork 15
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 simple Ruby client example #19
Conversation
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'll add a comment about streaming approach later.)
We need to implement Because |
For |
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Thanks for the review @kou. How about I remove the streaming example for now? |
@amoeba if you remove the streaming example, can you copy it into a Gist and we can link to that from the README for future reference? |
Sure. I put it in a gist. While it works, I don't think it actually streams (I think it buffers the entire response in memory before moving on) but it's there. |
Ok, thanks! I'll leave it to you to link it from the readme (or not) if you think it's worth it. |
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.
We can remove the current streaming implementation and merge this.
BTW, the current implementation isn't streaming because res.read_body
blocks until all response data received. We need to rewrite it entirely. So I think that we don't need to refer the Gist from README.
Co-authored-by: Sutou Kouhei <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Thanks @kou.
Yep, I agree. I should've made that more clear above. I just merged your changes and am going to give them a quick test. |
Co-authored-by: Ian Cook <[email protected]>
Thanks @ianmcook. This is good to go now. I tested locally and get this when I run it at the recently-merged server example:
|
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 successfully tested this against all the server examples.
While testing, I temporarily added this code at the bottom to write the table to a file:
output_file_path = "output-ruby.arrow"
output_stream = Arrow::FileOutputStream.new(output_file_path, false)
writer = Arrow::RecordBatchFileWriter.new(output_stream, schema)
writer.write_table(table)
writer.close
output_stream.close
I confirmed that all the resulting files were valid.
FYI: We can write it as only |
Fixes apache/arrow#40478.
Goes with open PR for Ruby server example #17.
Hi @kou, do you want to have a look? These Ruby APIs aren't familiar to me and I haven't used red-arrow before so I imagine this can be improved.
The second example is nonsense right now and doesn't work but I'll look at this again tomorrow.Edit: With a fresh set of eyes, I think I fixed the streaming example, or at least made it run.So far I've just tested this against the Ruby server PR and not others.