kou commented on code in PR #51277:
URL: https://github.com/apache/arrow/pull/51277#discussion_r4066897598
##########
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:
Could you use `...new_arrow_full()` instead of
`...new_arrow_with_properties()` like existing functions?
##########
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:
Could you add the `source` property as a construct only property and set it
by constructor?
##########
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,
+ GParquetReaderProperties
*properties,
+ GError **error);
+
+GPARQUET_AVAILABLE_IN_26_0
+GParquetArrowFileReader *
+gparquet_arrow_file_reader_new_path_with_properties(const gchar *path,
+ GParquetReaderProperties
*properties,
+ GError **error);
+
GPARQUET_AVAILABLE_IN_23_0
Review Comment:
ditto.
##########
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:
Could you use "nonexistent" not "missing" because existing code uses
"nonexistent"?
##########
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:
Why do we need them in this test?
--
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]
