emecii commented on code in PR #51277:
URL: https://github.com/apache/arrow/pull/51277#discussion_r4089585735
##########
c_glib/parquet-glib/arrow-file-reader.h:
##########
@@ -43,6 +78,18 @@ GPARQUET_AVAILABLE_IN_0_11
GParquetArrowFileReader *
gparquet_arrow_file_reader_new_path(const gchar *path, GError **error);
+GPARQUET_AVAILABLE_IN_26_0
+GParquetArrowFileReader *
+gparquet_arrow_file_reader_new_arrow_with_properties(GArrowSeekableInputStream
*source,
Review Comment:
Renamed both constructors to `gparquet_arrow_file_reader_new_arrow_full()`
and `gparquet_arrow_file_reader_new_path_full()` in
bd192d6070a7324b260ecda3c3b0be28f415dbef, including their documentation and
error tags. Rebuilt introspection and verified both Ruby overloads.
##########
c_glib/parquet-glib/arrow-file-reader.cpp:
##########
@@ -181,6 +349,63 @@ gparquet_arrow_file_reader_new_path(const gchar *path,
GError **error)
}
}
+/**
+ * gparquet_arrow_file_reader_new_arrow_with_properties:
+ * @source: Arrow source to be read.
+ * @properties: (nullable): Reader properties or %NULL for the defaults.
+ * @error: (nullable): Return location for a #GError or %NULL.
+ *
+ * The reader copies @properties at construction. Later changes to @properties
+ * do not affect the reader. The native source is retained by the reader.
+ *
+ * Returns: (nullable): A newly created #GParquetArrowFileReader.
+ *
+ * Since: 26.0.0
+ */
+GParquetArrowFileReader *
+gparquet_arrow_file_reader_new_arrow_with_properties(GArrowSeekableInputStream
*source,
+ GParquetReaderProperties
*properties,
+ GError **error)
+{
+ auto reader = open_reader_with_properties(
+ garrow_seekable_input_stream_get_raw(source),
+ properties,
+ error,
+ "[parquet][arrow][file-reader][new-arrow-with-properties]");
+ if (reader) {
+ auto priv = GPARQUET_ARROW_FILE_READER_GET_PRIVATE(reader);
+ priv->source = GARROW_SEEKABLE_INPUT_STREAM(g_object_ref(source));
Review Comment:
Added the construct-only `source` GObject property in
bd192d6070a7324b260ecda3c3b0be28f415dbef. The constructor now passes it through
`g_object_new()`. The native C check verifies the property flags, source
retention after caller release, and final release when the reader is destroyed.
##########
c_glib/test/parquet/test-arrow-file-reader.rb:
##########
@@ -39,6 +39,80 @@ def setup
end
end
+ sub_test_case(".new with properties") do
+ data("path" => :path, "stream" => :stream)
+ test("read") do |source_type|
+ properties = Parquet::ReaderProperties.new
+ properties.enable_buffered_stream
+ properties.buffer_size = 4096
+ source = if source_type == :path
+ @file.path
+ else
+ Arrow::FileInputStream.new(@file.path)
+ end
+ reader = Parquet::ArrowFileReader.new(source, properties)
+ begin
+ # The reader owns a copy of the properties and the native source.
+ properties.disable_buffered_stream
+ properties.buffer_size = 0
+ properties.unref
Review Comment:
Those lines were intended to check that construction copies the properties
and retains the source: releasing the source wrapper otherwise closes the
underlying stream. I separated them into named `copies properties` and `retains
source` tests in bd192d6070a7324b260ecda3c3b0be28f415dbef, leaving the basic
read test focused on table/row-group results. The native I/O probe also
verifies the original buffering settings after property mutation/destruction.
Validation passed: 16 focused GLib tests, all 73 GLib Parquet tests, all 18 Red
Parquet tests, and 10 native I/O cases.
##########
c_glib/test/parquet/test-arrow-file-reader.rb:
##########
@@ -39,6 +39,80 @@ def setup
end
end
+ sub_test_case(".new with properties") do
+ data("path" => :path, "stream" => :stream)
+ test("read") do |source_type|
+ properties = Parquet::ReaderProperties.new
+ properties.enable_buffered_stream
+ properties.buffer_size = 4096
+ source = if source_type == :path
+ @file.path
+ else
+ Arrow::FileInputStream.new(@file.path)
+ end
+ reader = Parquet::ArrowFileReader.new(source, properties)
+ begin
+ # The reader owns a copy of the properties and the native source.
+ properties.disable_buffered_stream
+ properties.buffer_size = 0
+ properties.unref
+ source.unref if source_type == :stream
+ assert_equal(@table, reader.read_table)
+ assert_equal(build_table("a" => @a_array.slice(1, 1),
+ "b" => @b_array.slice(1, 1)),
+ reader.read_row_group(1))
+ ensure
+ reader.close
+ reader.unref
+ end
+ end
+
+ data("path" => :path, "stream" => :stream)
+ test("default properties") do |source_type|
+ source = if source_type == :path
+ @file.path
+ else
+ Arrow::FileInputStream.new(@file.path)
+ end
+ reader = Parquet::ArrowFileReader.new(source, nil)
+ begin
+ assert_equal(@table, reader.read_table)
+ ensure
+ reader.close
+ reader.unref
+ source.unref if source_type == :stream
+ end
+ end
+
+ test("missing path") do
+ properties = Parquet::ReaderProperties.new
+ assert_raise(Arrow::Error::Io) do
+ Parquet::ArrowFileReader.new("#{@file.path}.missing", properties)
Review Comment:
Changed both the test name and path suffix to `nonexistent` in
bd192d6070a7324b260ecda3c3b0be28f415dbef; the error-path test passes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
