You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "scovich (via GitHub)" <gi...@apache.org> on 2025/08/22 03:03:49 UTC
[PR] [Variant] Support read-only metadata builders [arrow-rs]
scovich opened a new pull request, #8208:
URL: https://github.com/apache/arrow-rs/pull/8208
# Which issue does this PR close?
- Closes https://github.com/apache/arrow-rs/issues/8152
# Rationale for this change
When manipulating existing variant values (unshredding, removing fields, etc), the metadata column is already defined and already contains all necessary field ids. In fact, defining new/different field ids would require rewriting the bytes of those already-encoded variant values. We need a way to build variant values that rely on an existing metadata dictionary.
# What changes are included in this PR?
* `MetadataBuilder` is now a trait, and most methods that work with metadata builders now take `&mut dyn MetadataBuilder` instead of `&mut MetadataBuilder`.
* The old `MetadataBuilder` struct is now `BasicMetadataBuilder` that implements `MetadataBuilder`
* Define a `ReadOnlyMetadataBuilder` that wraps a `VariantMetadata` and which also implements `MetadataBuilder`
* Update the `try_binary_search_range_by` helper method to be more general, so we can define an efficient `VariantMetadata::get_entry` that returns the field id for a given field name.
# Are these changes tested?
Existing tests cover the basic metadata builder. New tests added to cover the read-only metadata builder.
# Are there any user-facing changes?
The renamed `BasicMetadataBuilder` (breaking), the new `MetadataBuilder` trait (breaking), and the new `ReadOnlyMetadataBuilder`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2295784217
##########
parquet-variant/src/builder.rs:
##########
@@ -431,13 +433,97 @@ impl ValueBuilder {
}
}
+/// A trait for building variant metadata dictionaries, to be used in conjunction with a
+/// [`ValueBuilder`]. The trait provides methods for managing field names and their IDs, as well as
+/// rolling back a failed builder operation that might have created new field ids.
+pub trait MetadataBuilder: std::fmt::Debug {
+ /// Attempts to register a field name, returning the corresponding (possibly newly-created)
+ /// field id on success. Attempting to register the same field name twice will _generally_
+ /// produce the same field id both times, but the variant spec does not actually require it.
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError>;
+
+ /// Retrieves the field name for a given field id, which must be less than
+ /// [`Self::num_field_names`]. Panics if the field id is out of bounds.
+ fn field_name(&self, field_id: usize) -> &str;
+
+ /// Returns the number of field names stored in this metadata builder. Any number less than this
+ /// is a valid field id. The builder can be reverted back to this size later on (discarding any
+ /// newer/higher field ids) by calling [`Self::truncate_field_names`].
+ fn num_field_names(&self) -> usize;
+
+ /// Reverts the field names to a previous size, discarding any newly out of bounds field ids.
+ fn truncate_field_names(&mut self, new_size: usize);
+}
+
+impl MetadataBuilder for BasicMetadataBuilder {
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError> {
+ Ok(self.upsert_field_name(field_name))
+ }
+ fn field_name(&self, field_id: usize) -> &str {
+ self.field_name(field_id)
+ }
+ fn num_field_names(&self) -> usize {
+ self.num_field_names()
+ }
+ fn truncate_field_names(&mut self, new_size: usize) {
+ self.field_names.truncate(new_size)
+ }
+}
+
+/// A metadata builder that cannot register new field names, and merely returns the field id
+/// associated with a known field name. This is useful for variant unshredding operations, where the
+/// metadata column is fixed and -- per variant shredding spec -- already contains all field names
+/// from the typed_value column. It is also useful when projecting a subset of fields from a variant
+/// object value, since the bytes can be copied across directly without re-encoding their field ids.
+#[derive(Debug)]
+pub struct ReadOnlyMetadataBuilder<'m> {
+ metadata: VariantMetadata<'m>,
+ known_field_names: HashMap<&'m str, u32>,
+}
+
+impl<'m> ReadOnlyMetadataBuilder<'m> {
+ /// Creates a new read-only metadata builder from the given metadata dictionary.
+ pub fn new(metadata: VariantMetadata<'m>) -> Self {
+ Self {
+ metadata,
+ known_field_names: HashMap::new(),
+ }
+ }
+}
+
+impl MetadataBuilder for ReadOnlyMetadataBuilder<'_> {
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError> {
+ if let Some(field_id) = self.known_field_names.get(field_name) {
+ return Ok(*field_id);
+ }
+
+ let Some((field_id, field_name)) = self.metadata.get_entry(field_name) else {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Field name '{field_name}' not found in metadata dictionary"
+ )));
+ };
+
+ self.known_field_names.insert(field_name, field_id);
+ Ok(field_id)
+ }
+ fn field_name(&self, field_id: usize) -> &str {
+ &self.metadata[field_id]
+ }
+ fn num_field_names(&self) -> usize {
+ self.metadata.len()
+ }
+ fn truncate_field_names(&mut self, new_size: usize) {
+ debug_assert_eq!(self.metadata.len(), new_size);
+ }
+}
+
/// Builder for constructing metadata for [`Variant`] values.
///
/// This is used internally by the [`VariantBuilder`] to construct the metadata
///
/// You can use an existing `Vec<u8>` as the metadata buffer by using the `from` impl.
#[derive(Default, Debug)]
-struct MetadataBuilder {
+struct BasicMetadataBuilder {
Review Comment:
I was thinking that Basic is a bit generic and it is somewhat unclear what the difference between a "ReadOnly" builder and a "Basic" builder are.
Here are some other ideas for names:
* `WriteableMetadataBuilder` -- can update the metadata (this is my preference)
* `OwnedMetadataBuilder` --- the builder owns the underlying structures (can thus can make changes)
##########
parquet-variant/src/builder.rs:
##########
@@ -431,13 +433,97 @@ impl ValueBuilder {
}
}
+/// A trait for building variant metadata dictionaries, to be used in conjunction with a
+/// [`ValueBuilder`]. The trait provides methods for managing field names and their IDs, as well as
+/// rolling back a failed builder operation that might have created new field ids.
+pub trait MetadataBuilder: std::fmt::Debug {
+ /// Attempts to register a field name, returning the corresponding (possibly newly-created)
+ /// field id on success. Attempting to register the same field name twice will _generally_
+ /// produce the same field id both times, but the variant spec does not actually require it.
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError>;
+
+ /// Retrieves the field name for a given field id, which must be less than
+ /// [`Self::num_field_names`]. Panics if the field id is out of bounds.
+ fn field_name(&self, field_id: usize) -> &str;
+
+ /// Returns the number of field names stored in this metadata builder. Any number less than this
+ /// is a valid field id. The builder can be reverted back to this size later on (discarding any
+ /// newer/higher field ids) by calling [`Self::truncate_field_names`].
+ fn num_field_names(&self) -> usize;
+
+ /// Reverts the field names to a previous size, discarding any newly out of bounds field ids.
+ fn truncate_field_names(&mut self, new_size: usize);
+}
+
+impl MetadataBuilder for BasicMetadataBuilder {
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError> {
+ Ok(self.upsert_field_name(field_name))
+ }
+ fn field_name(&self, field_id: usize) -> &str {
+ self.field_name(field_id)
+ }
+ fn num_field_names(&self) -> usize {
+ self.num_field_names()
+ }
+ fn truncate_field_names(&mut self, new_size: usize) {
+ self.field_names.truncate(new_size)
+ }
+}
+
+/// A metadata builder that cannot register new field names, and merely returns the field id
+/// associated with a known field name. This is useful for variant unshredding operations, where the
+/// metadata column is fixed and -- per variant shredding spec -- already contains all field names
+/// from the typed_value column. It is also useful when projecting a subset of fields from a variant
+/// object value, since the bytes can be copied across directly without re-encoding their field ids.
+#[derive(Debug)]
+pub struct ReadOnlyMetadataBuilder<'m> {
+ metadata: VariantMetadata<'m>,
+ known_field_names: HashMap<&'m str, u32>,
Review Comment:
it might be good to comment here that this structure does not include all metadata fields, it is more like a cache of known field
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
using `&dyn` here I think means all accesses now need a level of indirection (and I think we can see this slowdown of a few percent in the benchmarks)
I wonder if (maybe as a follow on PR) we can consider making parent state generic and squeezing that performance back. It may also be premature optimization for now
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296036836
##########
parquet-variant/src/builder.rs:
##########
@@ -431,13 +433,97 @@ impl ValueBuilder {
}
}
+/// A trait for building variant metadata dictionaries, to be used in conjunction with a
+/// [`ValueBuilder`]. The trait provides methods for managing field names and their IDs, as well as
+/// rolling back a failed builder operation that might have created new field ids.
+pub trait MetadataBuilder: std::fmt::Debug {
+ /// Attempts to register a field name, returning the corresponding (possibly newly-created)
+ /// field id on success. Attempting to register the same field name twice will _generally_
+ /// produce the same field id both times, but the variant spec does not actually require it.
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError>;
+
+ /// Retrieves the field name for a given field id, which must be less than
+ /// [`Self::num_field_names`]. Panics if the field id is out of bounds.
+ fn field_name(&self, field_id: usize) -> &str;
+
+ /// Returns the number of field names stored in this metadata builder. Any number less than this
+ /// is a valid field id. The builder can be reverted back to this size later on (discarding any
+ /// newer/higher field ids) by calling [`Self::truncate_field_names`].
+ fn num_field_names(&self) -> usize;
+
+ /// Reverts the field names to a previous size, discarding any newly out of bounds field ids.
+ fn truncate_field_names(&mut self, new_size: usize);
+}
+
+impl MetadataBuilder for BasicMetadataBuilder {
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError> {
+ Ok(self.upsert_field_name(field_name))
+ }
+ fn field_name(&self, field_id: usize) -> &str {
+ self.field_name(field_id)
+ }
+ fn num_field_names(&self) -> usize {
+ self.num_field_names()
+ }
+ fn truncate_field_names(&mut self, new_size: usize) {
+ self.field_names.truncate(new_size)
+ }
+}
+
+/// A metadata builder that cannot register new field names, and merely returns the field id
+/// associated with a known field name. This is useful for variant unshredding operations, where the
+/// metadata column is fixed and -- per variant shredding spec -- already contains all field names
+/// from the typed_value column. It is also useful when projecting a subset of fields from a variant
+/// object value, since the bytes can be copied across directly without re-encoding their field ids.
+#[derive(Debug)]
+pub struct ReadOnlyMetadataBuilder<'m> {
+ metadata: VariantMetadata<'m>,
+ known_field_names: HashMap<&'m str, u32>,
+}
+
+impl<'m> ReadOnlyMetadataBuilder<'m> {
+ /// Creates a new read-only metadata builder from the given metadata dictionary.
+ pub fn new(metadata: VariantMetadata<'m>) -> Self {
+ Self {
+ metadata,
+ known_field_names: HashMap::new(),
+ }
+ }
+}
+
+impl MetadataBuilder for ReadOnlyMetadataBuilder<'_> {
+ fn try_upsert_field_name(&mut self, field_name: &str) -> Result<u32, ArrowError> {
+ if let Some(field_id) = self.known_field_names.get(field_name) {
+ return Ok(*field_id);
+ }
+
+ let Some((field_id, field_name)) = self.metadata.get_entry(field_name) else {
+ return Err(ArrowError::InvalidArgumentError(format!(
+ "Field name '{field_name}' not found in metadata dictionary"
+ )));
+ };
+
+ self.known_field_names.insert(field_name, field_id);
+ Ok(field_id)
+ }
+ fn field_name(&self, field_id: usize) -> &str {
+ &self.metadata[field_id]
+ }
+ fn num_field_names(&self) -> usize {
+ self.metadata.len()
+ }
+ fn truncate_field_names(&mut self, new_size: usize) {
+ debug_assert_eq!(self.metadata.len(), new_size);
+ }
+}
+
/// Builder for constructing metadata for [`Variant`] values.
///
/// This is used internally by the [`VariantBuilder`] to construct the metadata
///
/// You can use an existing `Vec<u8>` as the metadata buffer by using the `from` impl.
#[derive(Default, Debug)]
-struct MetadataBuilder {
+struct BasicMetadataBuilder {
Review Comment:
Went with `WritableMetadataBuilder`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296013459
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
Yeah I was trying to avoid the "pollution" that generics would cause, which is clearly visible in my first attempt:
* https://github.com/apache/arrow-rs/pull/7915
But we may need to go generic now, because `VariantArrayVariantBuilder::finish` needs access to `BasicMetadataBuilder::finish` in order to work correctly, and that's unavailable because the parent state has upcast it to `&mut dyn MetadataBuilder`.
I initially reached for `std::any::AsAny`, but it turns out that types with non-static lifetimes cannot implement `std::any::Any` because the lifetime information would be lost.
In order to get the latest merge with upstream to compile, I had to define a new `MetadataBuilder::finish` trait method to expose the underlying `BasicMetadataBuilder::finish`, but that also forced a no-op `ReadOnlyMetadataBuilder::finish`.
Honestly, the generic mess gets messy enough (including a bunch of methods that are only defined for specific combinations of types) that I lean strongly toward keeping the `dyn` indirection unless we're _really_ certain we need it.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296612336
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
> Honestly, the generic mess is messy enough that I lean strongly toward keeping the dyn indirection unless we're really certain we need generics for performance or functionality reasons.
I think this is a wise strategy. Let's wait for some more "end to end" type benchmarks (like reading/writing JSON to arrays, or shredding variants) and see if we need to try and squeeze a few more percent out
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296027615
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
A related issue we'll hit shortly: `VariantArrayBuilder` is currently hard-wired to use a `BasicMetadataBuilder` that produces a new metadata column.
Once we need a variant array builder that uses-and-preserves an existing metadata column (e.g. for unshredding), we'll be forced to decide whether we want to make `VariantArrayBuilder` generic over `M: MetadataBuilder` or just define a second builder class.
I suspect that choice will ultimately decide the generic-vs-not question.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3214104532
The docs error will self-resolve once https://github.com/apache/arrow-rs/pull/8206 merges:
```
error: public documentation for `MetadataBuilder` links to private item `ValueBuilder`
--> parquet-variant/src/builder.rs:437:7
|
437 | /// [`ValueBuilder`]. The trait provides methods for managing field names and their IDs, as well as
| ^^^^^^^^^^^^ this item is private
|
```
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "klion26 (via GitHub)" <gi...@apache.org>.
klion26 commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3222682748
Sorry for the late reply, I just came back from out yesterday and just saw the notifications. There is no more comment from me.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3219907590
Thanks again @scovich
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3216707220
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group main metadata-builder-trait
----- ---- ----------------------
bench_validate_complex_object 1.00 228.3±1.68µs ? ?/sec 1.00 229.3±0.69µs ? ?/sec
bench_validate_large_nested_list 1.00 19.4±0.03ms ? ?/sec 1.00 19.5±0.05ms ? ?/sec
bench_validate_large_object 1.00 54.9±0.09ms ? ?/sec 1.00 54.9±0.09ms ? ?/sec
```
</p>
</details>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296027615
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
A related issue we'll hit shortly: `VariantArrayBuilder` is currently hard-wired to use a `BasicMetadataBuilder` that produces a new metadata column.
Once we need a variant array builder that reuses an existing metadata column (e.g. for unshredding), we'll be forced to decide whether we want to make `VariantArrayBuilder` generic over `M: MetadataBuilder` or just define a second builder class.
I suspect that choice will ultimately decide the generic-vs-not question.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3216699833
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1014-gcp #15~24.04.1-Ubuntu SMP Fri Jul 25 23:26:08 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing metadata-builder-trait (1a4493b97dd7726edde8cf65e2fabab82f5f354f) to cec24a05ee41a3940d83076129d594ad5e2354f6 [diff](https://github.com/apache/arrow-rs/compare/cec24a05ee41a3940d83076129d594ad5e2354f6..1a4493b97dd7726edde8cf65e2fabab82f5f354f)
BENCH_NAME=variant_kernels
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench variant_kernels
BENCH_FILTER=
BENCH_BRANCH_NAME=metadata-builder-trait
Results will be posted here when complete
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3216703315
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group main metadata-builder-trait
----- ---- ----------------------
batch_json_string_to_variant json_list 8k string 1.00 25.9±0.15ms ? ?/sec 1.02 26.4±0.09ms ? ?/sec
batch_json_string_to_variant random_json(2633 bytes per document) 1.00 298.8±6.38ms ? ?/sec 1.02 306.3±6.90ms ? ?/sec
batch_json_string_to_variant repeated_struct 8k string 1.00 7.7±0.04ms ? ?/sec 1.02 7.9±0.16ms ? ?/sec
variant_get_primitive 1.00 1077.9±3.16µs ? ?/sec 1.02 1102.7±4.00µs ? ?/sec
```
</p>
</details>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3216703347
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1014-gcp #15~24.04.1-Ubuntu SMP Fri Jul 25 23:26:08 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing metadata-builder-trait (1a4493b97dd7726edde8cf65e2fabab82f5f354f) to cec24a05ee41a3940d83076129d594ad5e2354f6 [diff](https://github.com/apache/arrow-rs/compare/cec24a05ee41a3940d83076129d594ad5e2354f6..1a4493b97dd7726edde8cf65e2fabab82f5f354f)
BENCH_NAME=variant_builder
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench variant_builder
BENCH_FILTER=
BENCH_BRANCH_NAME=metadata-builder-trait
Results will be posted here when complete
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3216706346
🤖: Benchmark completed
<details><summary>Details</summary>
<p>
```
group main metadata-builder-trait
----- ---- ----------------------
bench_extend_metadata_builder 1.00 53.4±2.28ms ? ?/sec 1.03 55.1±3.44ms ? ?/sec
bench_object_field_names_reverse_order 1.00 19.6±0.46ms ? ?/sec 1.04 20.3±0.48ms ? ?/sec
bench_object_list_partially_same_schema 1.00 1218.7±15.41µs ? ?/sec 1.04 1266.7±14.56µs ? ?/sec
bench_object_list_same_schema 1.00 24.4±0.22ms ? ?/sec 1.04 25.3±0.19ms ? ?/sec
bench_object_list_unknown_schema 1.00 13.1±0.16ms ? ?/sec 1.03 13.6±0.16ms ? ?/sec
bench_object_partially_same_schema 1.00 3.2±0.01ms ? ?/sec 1.02 3.3±0.01ms ? ?/sec
bench_object_same_schema 1.00 37.0±0.10ms ? ?/sec 1.02 37.7±0.12ms ? ?/sec
bench_object_unknown_schema 1.00 15.9±0.02ms ? ?/sec 1.01 16.1±0.05ms ? ?/sec
iteration/unvalidated_fallible_iteration 1.00 2.6±0.01ms ? ?/sec 1.01 2.7±0.01ms ? ?/sec
iteration/validated_iteration 1.00 49.5±0.28µs ? ?/sec 1.01 49.8±0.11µs ? ?/sec
validation/unvalidated_construction 1.00 6.7±0.06µs ? ?/sec 1.00 6.7±0.02µs ? ?/sec
validation/validated_construction 1.00 60.6±0.14µs ? ?/sec 1.02 61.5±0.21µs ? ?/sec
validation/validation_cost 1.00 53.8±0.11µs ? ?/sec 1.05 56.5±0.12µs ? ?/sec
```
</p>
</details>
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3216706377
🤖 `./gh_compare_arrow.sh` [Benchmark Script](https://github.com/alamb/datafusion-benchmarking/blob/main/scripts/gh_compare_arrow.sh) Running
Linux aal-dev 6.14.0-1014-gcp #15~24.04.1-Ubuntu SMP Fri Jul 25 23:26:08 UTC 2025 x86_64 x86_64 x86_64 GNU/Linux
Comparing metadata-builder-trait (1a4493b97dd7726edde8cf65e2fabab82f5f354f) to cec24a05ee41a3940d83076129d594ad5e2354f6 [diff](https://github.com/apache/arrow-rs/compare/cec24a05ee41a3940d83076129d594ad5e2354f6..1a4493b97dd7726edde8cf65e2fabab82f5f354f)
BENCH_NAME=variant_validation
BENCH_COMMAND=cargo bench --features=arrow,async,test_common,experimental --bench variant_validation
BENCH_FILTER=
BENCH_BRANCH_NAME=metadata-builder-trait
Results will be posted here when complete
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#issuecomment-3216723222
(this PR also needs an update to resolve some merge conflcits)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296037432
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
I suppose we could also avoid generics altogether by defining our own versions of `Any` and `AsAny` that preserve lifetime info... but they would become part of the public arrow-rs API which seems awkward.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "scovich (via GitHub)" <gi...@apache.org>.
scovich commented on code in PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208#discussion_r2296013459
##########
parquet-variant/src/builder.rs:
##########
@@ -589,14 +675,14 @@ enum ParentState<'a> {
Variant {
value_builder: &'a mut ValueBuilder,
saved_value_builder_offset: usize,
- metadata_builder: &'a mut MetadataBuilder,
+ metadata_builder: &'a mut dyn MetadataBuilder,
Review Comment:
Yeah I was trying to avoid the "pollution" that generics would cause, which is clearly visible in my first attempt:
* https://github.com/apache/arrow-rs/pull/7915
It's not just `ParentState`, but also `ListBuilder`, `ObjectBuilder` and even the `VariantBuilderExt` trait. Plus it requires a bunch of methods that are only defined for specific combinations of types.
But we may need to go generic now, because `VariantArrayVariantBuilder::finish` needs access to `BasicMetadataBuilder::finish` in order to work correctly, and that's unavailable because the parent state has upcast it to `&mut dyn MetadataBuilder`.
I initially reached for `std::any::AsAny`, but it turns out that types with non-static lifetimes cannot implement `std::any::Any` because the lifetime information would be lost.
In order to get the latest merge with upstream to compile, I had to define a new `MetadataBuilder::finish` trait method to expose the underlying `BasicMetadataBuilder::finish`, but that also forced a no-op `ReadOnlyMetadataBuilder::finish`.
Honestly, the generic mess is messy enough that I lean strongly toward keeping the `dyn` indirection unless we're _really_ certain we need generics for performance or functionality reasons.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Variant] Support read-only metadata builders [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #8208:
URL: https://github.com/apache/arrow-rs/pull/8208
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org