-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-48680: [GLib][Ruby] Add CSVWriter #48681
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
base: main
Are you sure you want to change the base?
Conversation
|
|
c_glib/arrow-glib/writer.cpp
Outdated
| spec = g_param_spec_string("null-string", | ||
| "Null string", | ||
| "The string to write for null values", | ||
| "", |
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.
| "", | |
| write_options.null_string.c_str(), |
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 always find this difficult in C++, so I wonder, is that safe? The data for the string will be owned by the write_options variable, right? And when that goes out of scope, the string could potentially be gone? I guess it works because it is a constant somewhere.
c_glib/arrow-glib/writer.cpp
Outdated
| spec = g_param_spec_string("eol", | ||
| "EOL", | ||
| "The end of line character to use for ending rows", | ||
| "\n", |
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.
| "\n", | |
| write_options.eol.c_str(), |
|
|
||
| def test_delimiter | ||
| assert_equal(44, @options.delimiter) # 44 is the ASCII code for comma | ||
| @options.delimiter = ";".ord |
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 use String, right?
| @options.delimiter = ";".ord | |
| @options.delimiter = ";" |
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.
No, this is implemented in the Ruby wrapper. It can't be used in the tests for the GLib wrapper, right?
| options = Arrow::CSVWriteOptions.new | ||
| options.include_header = false | ||
| options.delimiter = ";".ord | ||
| options.quoting_style = Arrow::CSVQuotingStyle::NONE | ||
| options.null_string = "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.
We can use
options = {
include_header: false,
delimiter: ";",
quoting_style: :none,
null_string: "NULL,
}by defining Arrow::CSVWriteOptions.try_convert:
module Arrow
class CSVWriteOptions
class << self
def try_convert(value)
case value
when Hash
options = new
value.each do |k, v|
options.public_send("#{k}=", value)
end
options
else
nil
end
end
end
end
endThere 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.
This is good idea! I assume you meant to comment on the test for the Ruby wrapper, not GLib though.
Rationale for this change
Using Arrow for writing CSVs could potentially give a lot better performance than using Ruby, but the CSV writer is not available.
What changes are included in this PR?
This adds a
CSVWriterand options class to the GLib wrapper.Are these changes tested?
Yes, with Ruby unit tests.
Are there any user-facing changes?
Yes, new classes.