GH-36010: [GLib][Ruby][Parquet] Add buffered reader properties by emecii · Pull Request #51277 · apache/arrow

emecii · 2026-09-10T07:19:03Z

Rationale for this change

GH-36010 requests Ruby bindings for parquet::ReaderProperties so callers can select buffered Parquet streams and configure their buffer size.

What changes are included in this PR?

Add GParquetReaderProperties with buffered-stream controls, buffer size, and independent pre_buffer get/set backed by parquet::ArrowReaderProperties.
Add new_arrow_full / new_path_full constructors accepting nullable properties. Introspection exposes Parquet::ArrowFileReader.new(source_or_path, properties) without a Ruby adapter.
Copy both native properties at construction and retain stream sources through a construct-only GObject property. Path and stream constructors share open_reader() and create the wrapper through gparquet_arrow_file_reader_new_raw(). Existing C signatures remain unchanged. The C++ raw helper uses a defaulted source argument: one-argument source calls still compile, but its previous one-argument binary symbol is replaced.
Preserve pre-buffering by default, including when buffered streams are enabled. Callers explicitly set properties.pre_buffer = false to avoid whole-column pre-buffering. Documentation explains that page reads and decoded data can exceed the configured buffer size.

Are these changes tested?

September 27 validation on macOS arm64 with rebuilt Debug GLib/introspection against Arrow/Parquet 26.0.0-SNAPSHOT:

Focused GLib reader/properties: 13 tests, 20 assertions; full GLib Parquet: 70 tests, 77 assertions; full Red Parquet: 16 tests, 20 assertions. All pass. Redundant tests were removed as requested in review.
An instrumented native source through Ruby passes 14 real table/row-group reads covering defaults, explicit pre-buffering, two buffer sizes, equal decoded data, and property snapshots. These are I/O checks, not RSS measurements or hard read-size caps.
Native C client compiled with -Wall -Wextra -Werror: constructor calls, property access, source lifetime, and real reads pass. Both one- and two-argument C++ raw helper calls compile (unused-parameter warnings in existing Arrow headers suppressed).
Clang-format 18.1.8, RuboCop 1.71.0, Ruby syntax, diff checks, and the committed-tree Apache RAT audit pass.

Are there any user-facing changes?

GLib/Ruby callers can configure buffered Parquet reads and explicitly control pre-buffering. Existing default reads keep their prior I/O behavior.

AI assistance

OpenAI Codex generated the implementation, regression tests and local validation harnesses, and ran the checks described above.

GitHub Issue: [Ruby][Parquet] Implement the bindings of parquet::ReaderProperties #36010

Expose buffered stream controls and additive properties-aware reader constructors. Copy native properties and retain the input stream; disable whole-column read-ahead when buffered streams are selected. Generated-by: OpenAI Codex

Free the reader after the .open block so Windows can remove the memory-mapped temporary file without waiting for garbage collection. Generated-by: OpenAI Codex

emecii · 2026-09-10T07:46:51Z

Fixed the MinGW temporary-file cleanup failure in b39e1a5. The new path test passed its assertions but retained the memory-mapped reader after the .open block, causing Windows to reject Tempfile deletion. The test now explicitly releases the reader in ensure.

Validated with a native mapping check that fails before the change and passes afterward, the full local Red Parquet suite (18 tests, 24 assertions), and the GLib suite (2,247 tests, zero failures/errors; 173 unavailable-feature omissions). Ruby syntax, RuboCop, and RAT checks pass. The hosted Windows rerun is pending.

kou · 2026-09-21T22:20:08Z


+GPARQUET_AVAILABLE_IN_26_0
+GParquetArrowFileReader *
+gparquet_arrow_file_reader_new_arrow_with_properties(GArrowSeekableInputStream *source,


Could you use ...new_arrow_full() instead of ...new_arrow_with_properties() like existing functions?

Renamed both constructors to gparquet_arrow_file_reader_new_arrow_full() and gparquet_arrow_file_reader_new_path_full() in bd192d6, including their documentation and error tags. Rebuilt introspection and verified both Ruby overloads.

kou · 2026-09-21T22:20:12Z

+gparquet_arrow_file_reader_new_path_with_properties(const gchar *path,
+                                                    GParquetReaderProperties *properties,
+                                                    GError **error);
+
 GPARQUET_AVAILABLE_IN_23_0


kou · 2026-09-21T22:22:34Z

+    "[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));


Could you add the source property as a construct only property and set it by constructor?

Added the construct-only source GObject property in bd192d6. 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.

kou · 2026-09-21T22:27:09Z

+        # The reader owns a copy of the properties and the native source.
+        properties.disable_buffered_stream
+        properties.buffer_size = 0
+        properties.unref


Why do we need them in this test?

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 bd192d6, 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.

kou · 2026-09-21T22:28:14Z

+    test("missing path") do
+      properties = Parquet::ReaderProperties.new
+      assert_raise(Arrow::Error::Io) do
+        Parquet::ArrowFileReader.new("#{@file.path}.missing", properties)


Could you use "nonexistent" not "missing" because existing code uses "nonexistent"?

Changed both the test name and path suffix to nonexistent in bd192d6; the error-path test passes.

Use full constructor names and retain the source through a construct-only GObject property. Separate property-copy and source-lifetime regressions from normal reading, and use the existing nonexistent-path terminology. Generated-by: OpenAI Codex

emecii · 2026-09-24T03:37:41Z

The new-head MinGW job failed during Setup MSYS2: downloading mingw-w64-ucrt-x86_64-gts-0.7.6-3-any.pkg.tar.zst.sig from ftp2.osuosl.org timed out (less than 1 byte/sec for 10 seconds). All build/test steps were skipped. My job-rerun request returned HTTP 403 (Must have admin rights to Repository). Please retry the failed job when convenient.

kou · 2026-09-24T06:13:41Z

+    return GPARQUET_ARROW_FILE_READER(g_object_new(GPARQUET_TYPE_ARROW_FILE_READER,
+                                                   "arrow-file-reader",
+                                                   result->release(),
+                                                   "source",
+                                                   source_object,
+                                                   NULL));


Could you use gparquet_arrow_file_reader_new_raw()?

Updated in 3b468be: open_reader() now constructs the wrapper through gparquet_arrow_file_reader_new_raw(reader, source), which sets the construct-only source property. The existing one-argument raw constructor delegates with a null source. Native C checks pass for source retention and final release.

kou · 2026-09-24T06:15:12Z


+namespace {
+  GParquetArrowFileReader *
+  open_reader_with_properties(std::shared_ptr<arrow::io::RandomAccessFile> source,


Can we remove with_properties and use this for gparquet_arrow_file_reader_new_arrow() too?

Renamed the helper to open_reader() and reused it for both new_arrow() and new_arrow_full() in 3b468be. The legacy constructor retains its error tag and default properties. The native I/O probe confirms identical default reads; a separate C check verifies legacy source retention.

kou · 2026-09-24T06:18:54Z

+    if (parquet_properties.is_buffered_stream_enabled()) {
+      // Read-ahead would bypass the buffered stream by caching whole column chunks.
+      auto arrow_properties = parquet::default_arrow_reader_properties();
+      arrow_properties.set_pre_buffer(false);


Could you avoid this implicit configuration?

Could you add parquet::ArrowReaderProperties to GParquetReaderPropertiesPrivate_ so that users can set arrow_properties.set_pre_buffer manually?

Added parquet::ArrowReaderProperties to the private properties struct and exposed set_pre_buffer() / get_pre_buffer() in 3b468be. Buffered-stream toggles no longer change pre-buffering; Ruby callers explicitly use properties.pre_buffer = false. Both native properties are copied into the reader. Updated documentation and tests pass: 17 focused GLib tests, 74 GLib Parquet tests, 18 Red Parquet tests, and 14 native I/O cases covering defaults, independent settings and snapshots after mutation/destruction.

Expose independent pre-buffer controls backed by ArrowReaderProperties. Reuse open_reader for both stream constructors and construct readers through new_raw with the retained source. Generated-by: OpenAI Codex

kou · 2026-09-25T04:01:02Z


+namespace {
+  GParquetArrowFileReader *
+  open_reader(std::shared_ptr<arrow::io::RandomAccessFile> source,


Can we use this in gparquet_arrow_file_reader_new_path() too to remove duplicated code?

Updated in bce0231: the path constructor now shares open_reader(), and both native property accessors return pointers.

kou · 2026-09-25T04:03:11Z

 GParquetArrowFileReader *
 gparquet_arrow_file_reader_new_raw(parquet::arrow::FileReader *parquet_arrow_file_reader);
+GParquetArrowFileReader *
+gparquet_arrow_file_reader_new_raw(parquet::arrow::FileReader *parquet_arrow_file_reader,
+                                   GArrowSeekableInputStream *source);


Can we simplify them by the default argument?

Suggested change

GParquetArrowFileReader *

gparquet_arrow_file_reader_new_raw(parquet::arrow::FileReader *parquet_arrow_file_reader);

GParquetArrowFileReader *

gparquet_arrow_file_reader_new_raw(parquet::arrow::FileReader *parquet_arrow_file_reader,

GArrowSeekableInputStream *source);

GParquetArrowFileReader *

gparquet_arrow_file_reader_new_raw(parquet::arrow::FileReader *parquet_arrow_file_reader,

GArrowSeekableInputStream *source=nullptr);

Replaced the overloads with the defaulted source argument in bce0231. One-argument source calls still compile; the previous one-argument C++ binary symbol is replaced. The public C signatures are unchanged.

kou · 2026-09-25T04:07:49Z

+gparquet_reader_properties_get_raw(GParquetReaderProperties *properties)
+{
+  auto priv = GPARQUET_READER_PROPERTIES_GET_PRIVATE(properties);
+  return priv->properties;


Could you return a pointer instead of a value like garrow_read_options_get_raw()?

Suggested change

return priv->properties;

return &(priv->properties);

kou · 2026-09-25T04:08:18Z

+gparquet_reader_properties_get_arrow_raw(GParquetReaderProperties *properties)
+{
+  auto priv = GPARQUET_READER_PROPERTIES_GET_PRIVATE(properties);
+  return priv->arrow_properties;


Could you return a pointer instead of a value like garrow_read_options_get_dictionary_memo_raw()?

Suggested change

return priv->arrow_properties;

return &(priv->arrow_properties);

kou · 2026-09-25T04:10:25Z

+    test("retains source") do
+      properties = Parquet::ReaderProperties.new
+      source = Arrow::FileInputStream.new(@file.path)
+      reader = Parquet::ArrowFileReader.new(source, properties)
+      begin
+        assert_equal(source, reader.source)
+        source.unref
+        assert_equal(@table, reader.read_table)
+      ensure
+        reader.close
+        reader.unref
+      end
+    end


Could you remove this test? We can detect the "source isn't referred" problem by crash.

kou · 2026-09-25T04:13:10Z

+    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


Could you remove this test? This case is covered by other tests that don't pass properties explicitly.

kou · 2026-09-25T04:14:46Z

+  end
+
+  def test_buffered_stream
+    assert_false(@properties.buffered_stream_enabled?)


Could you use this for better failure message?

Suggested change

assert_false(@properties.buffered_stream_enabled?)

assert do

not @properties.buffered_stream_enabled?

end

kou · 2026-09-25T04:15:18Z

+
+  def test_buffered_stream
+    assert_false(@properties.buffered_stream_enabled?)
+    assert_true(@properties.pre_buffer?)


Why do you test pre_buffer? in this test? I think that test_pre_buffer covers pre_buffer? cases.

kou · 2026-09-25T04:15:51Z

+    assert_equal(16 * 1024, @properties.buffer_size)
+    @properties.buffer_size = 32 * 1024
+    assert_equal(32 * 1024, @properties.buffer_size)
+    assert_false(@properties.buffered_stream_enabled?)


Why do we need to check buffered_stream_enabled? in this test?

kou · 2026-09-25T04:17:23Z

+  sub_test_case(".open with properties") do
+    data("path" => :path, "stream" => :stream)
+    test("row groups") do |source_type|
+      properties = Parquet::ReaderProperties.new
+      properties.buffer_size = 4096
+      properties.enable_buffered_stream
+      properties.pre_buffer = false
+      assert_true(properties.buffered_stream_enabled?)
+      source = if source_type == :path
+                 @file.path
+               else
+                 Arrow::FileInputStream.new(@file.path)
+               end
+      reader = nil
+      begin
+        Parquet::ArrowFileReader.open(source, properties) do |opened_reader|
+          reader = opened_reader
+          properties.pre_buffer = true
+          properties.disable_buffered_stream
+          properties.buffer_size = 0
+          assert_equal([
+                         Arrow::Table.new(@schema, [[true]]),
+                         Arrow::Table.new(@schema, [[false]])
+                       ],
+                       reader.each_row_group.to_a)
+        end
+      ensure
+        # Release the memory-mapped path before Tempfile removes it on Windows.
+        reader&.unref
+        source.close if source_type == :stream
+      end
+    end
+  end


We don't need to add this test because this case is covered by GLib tests. If we add support Parquet::ArrowFileReader.open(source, buffer_size: ...) style API, we can add a test for the API.

Share the path constructor helper, return native property pointers, use the requested default source argument, and remove redundant tests following review. Generated-by: OpenAI Codex

kou

+1

emecii requested a review from kou as a code owner September 10, 2026 07:19

github-actions Bot added Component: Parquet Component: Ruby Component: GLib awaiting review Awaiting review labels Sep 10, 2026

apacheGH-36010: [Ruby][Parquet] Release mapped reader in buffered test

b39e1a5

Free the reader after the .open block so Windows can remove the memory-mapped temporary file without waiting for garbage collection. Generated-by: OpenAI Codex

kou reviewed Sep 21, 2026

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Sep 21, 2026

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 24, 2026

kou reviewed Sep 24, 2026

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 24, 2026

apacheGH-36010: [GLib][Parquet] Make reader pre-buffering explicit

3b468be

Expose independent pre-buffer controls backed by ArrowReaderProperties. Reuse open_reader for both stream constructors and construct readers through new_raw with the retained source. Generated-by: OpenAI Codex

github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Sep 24, 2026

kou reviewed Sep 25, 2026

View reviewed changes

github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Sep 25, 2026

apacheGH-36010: [GLib][Parquet] Simplify reader construction and tests

bce0231

Share the path constructor helper, return native property pointers, use the requested default source argument, and remove redundant tests following review. Generated-by: OpenAI Codex

github-actions Bot added awaiting change review Awaiting change review and removed Component: Ruby awaiting changes Awaiting changes labels Sep 27, 2026

kou approved these changes Sep 28, 2026

View reviewed changes

github-actions Bot removed the awaiting change review Awaiting change review label Sep 28, 2026

github-actions Bot added the awaiting merge Awaiting merge label Sep 28, 2026

kou merged commit dcdc129 into apache:main Sep 28, 2026
37 of 38 checks passed

kou removed the awaiting merge Awaiting merge label Sep 28, 2026

kou mentioned this pull request Sep 28, 2026

[Ruby][Parquet] Implement the bindings of parquet::ReaderProperties #36010

Closed

	return priv->arrow_properties;
	return &(priv->arrow_properties);

-    assert_false(@properties.buffered_stream_enabled?)
+    assert do
+      not @properties.buffered_stream_enabled?
+    end

Conversation

emecii commented Sep 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

AI assistance

Uh oh!

emecii commented Sep 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emecii commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

emecii commented Sep 10, 2026 •

edited

Loading