alamb commented on code in PR #7808:
URL: https://github.com/apache/arrow-rs/pull/7808#discussion_r2173675035
##########
parquet-variant/src/builder.rs:
##########
@@ -567,7 +564,8 @@ impl<'a> ListBuilder<'a> {
pub struct ObjectBuilder<'a, 'b> {
parent_buffer: &'a mut ValueBuffer,
metadata_builder: &'a mut MetadataBuilder,
- fields: BTreeMap<u32, usize>, // (field_id, offset)
+ fields: Vec<(u32, usize)>, // (field_id, offset)
+ field_id_to_index: HashMap<u32, usize>, // (field_id, index to `fields`)
Review Comment:
You might be able to use an `IndexSet` here to store the field_ids as well
as preserving order
##########
parquet-variant/benches/builder.rs:
##########
@@ -0,0 +1,389 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+extern crate parquet_variant;
+
+use criterion::*;
+
+use parquet_variant::VariantBuilder;
+use rand::{
+ distr::{uniform::SampleUniform, Alphanumeric},
+ rngs::ThreadRng,
+ Rng,
+};
+use std::{hint, ops::Range};
+
+fn random<T: SampleUniform + PartialEq + PartialOrd>(rng: &mut ThreadRng,
range: Range<T>) -> T {
+ rng.random_range::<T, _>(range)
+}
+
+// generates a string with a 50/50 chance whether it's a short or a long string
+fn random_string(rng: &mut ThreadRng) -> String {
+ let len = rng.random_range::<usize, _>(1..128);
+
+ rng.sample_iter(&Alphanumeric)
+ .take(len)
+ .map(char::from)
+ .collect()
+}
+
+// generates a string guaranteed to be longer than 64 bytes
+fn random_long_string(rng: &mut ThreadRng) -> String {
+ let len = rng.random_range::<usize, _>(65..200);
+
+ rng.sample_iter(&Alphanumeric)
+ .take(len)
+ .map(char::from)
+ .collect()
+}
+
+// Creates an object with field names inserted in reverse lexicographical order
+fn bench_object_field_names_reverse_order(c: &mut Criterion) {
+ c.bench_function("bench_object_field_names_reverse_order", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ let mut variant = VariantBuilder::new();
+ let mut object_builder = variant.new_object();
+
+ for i in 0..50_000 {
+ object_builder.insert(
+ format!("{}", 1000 - i).as_str(),
+ random_string(&mut rng).as_str(),
+ );
+ }
+
+ object_builder.finish();
+ hint::black_box(variant.finish());
+ })
+ });
+}
+
+// Creates objects with a homogenous schema (same field names)
+/*
+ {
+ name: String,
+ age: i32,
+ likes_cilantro: bool,
+ comments: Long string
+ dishes: Vec<String>
+ }
+*/
+fn bench_object_same_schema(c: &mut Criterion) {
+ c.bench_function("bench_object_same_schema", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ for _ in 0..25_000 {
+ let mut variant = VariantBuilder::new();
+ let mut object_builder = variant.new_object();
+ object_builder.insert("name", random_string(&mut
rng).as_str());
+ object_builder.insert("age", random::<u32>(&mut rng, 18..100)
as i32);
+ object_builder.insert("likes_cilantro", rng.random_bool(0.5));
+ object_builder.insert("comments", random_long_string(&mut
rng).as_str());
+
+ let mut inner_list_builder = object_builder.new_list("dishes");
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+
+ inner_list_builder.finish();
+ object_builder.finish();
+
+ hint::black_box(variant.finish());
+ }
+ })
+ });
+}
+
+// Creates a list of objects with the same schema (same field names)
+/*
+ {
+ name: String,
+ age: i32,
+ likes_cilantro: bool,
+ comments: Long string
+ dishes: Vec<String>
+ }
+*/
+fn bench_object_list_same_schema(c: &mut Criterion) {
+ c.bench_function("bench_object_list_same_schema", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ let mut variant = VariantBuilder::new();
+
+ let mut list_builder = variant.new_list();
+
+ for _ in 0..25_000 {
Review Comment:
I need to profile this to confirm, but I suspect a non trivial amount of the
benchmark time isspent actually generating the random strings
To ensure we are only benchmarking the VariantBuilder, I suggest
pre-generating a string table and then pick from them quasi-randomly --
something like
```rust
// make a table with 117 random strings (so it doesn't evenly divide into
25000)
let strings = (0..117).map(|_| random_string(&mut rng).collect::<Vec<_>>();
...
b.iter(|| {
let mut current_string = 0;
...
object_builder.insert("name", &strings[current_string]);
current_string = (current_string + 1) % strings.len();
..
```
##########
parquet-variant/src/builder.rs:
##########
@@ -233,29 +234,25 @@ impl ValueBuffer {
#[derive(Default)]
struct MetadataBuilder {
- field_name_to_id: BTreeMap<String, u32>,
- field_names: Vec<String>,
+ field_names: IndexSet<String>,
Review Comment:
it might also help here to add some clarification, like
```suggestion
// Field Names -- field_ids are assigned in insert order
// which is maintained by using an IndexSet
field_names: IndexSet<String>,
```
##########
parquet-variant/benches/builder.rs:
##########
@@ -0,0 +1,389 @@
+// Licensed to the Apache Software Foundation (ASF) under one
Review Comment:
This benchmarks name is workspace wide, so it is run like this:
```
cargo bench --bench builder
```
However there are many builders in the arrow repo so this may be ambiguous
I suggest we name this `variant_builder.rs` instead
##########
parquet-variant/benches/builder.rs:
##########
@@ -0,0 +1,389 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+extern crate parquet_variant;
+
+use criterion::*;
+
+use parquet_variant::VariantBuilder;
+use rand::{
+ distr::{uniform::SampleUniform, Alphanumeric},
+ rngs::ThreadRng,
+ Rng,
+};
+use std::{hint, ops::Range};
+
+fn random<T: SampleUniform + PartialEq + PartialOrd>(rng: &mut ThreadRng,
range: Range<T>) -> T {
+ rng.random_range::<T, _>(range)
+}
+
+// generates a string with a 50/50 chance whether it's a short or a long string
+fn random_string(rng: &mut ThreadRng) -> String {
+ let len = rng.random_range::<usize, _>(1..128);
+
+ rng.sample_iter(&Alphanumeric)
+ .take(len)
+ .map(char::from)
+ .collect()
+}
+
+// generates a string guaranteed to be longer than 64 bytes
+fn random_long_string(rng: &mut ThreadRng) -> String {
+ let len = rng.random_range::<usize, _>(65..200);
+
+ rng.sample_iter(&Alphanumeric)
+ .take(len)
+ .map(char::from)
+ .collect()
+}
+
+// Creates an object with field names inserted in reverse lexicographical order
+fn bench_object_field_names_reverse_order(c: &mut Criterion) {
+ c.bench_function("bench_object_field_names_reverse_order", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ let mut variant = VariantBuilder::new();
+ let mut object_builder = variant.new_object();
+
+ for i in 0..50_000 {
+ object_builder.insert(
+ format!("{}", 1000 - i).as_str(),
+ random_string(&mut rng).as_str(),
+ );
+ }
+
+ object_builder.finish();
+ hint::black_box(variant.finish());
+ })
+ });
+}
+
+// Creates objects with a homogenous schema (same field names)
+/*
+ {
+ name: String,
+ age: i32,
+ likes_cilantro: bool,
+ comments: Long string
+ dishes: Vec<String>
+ }
+*/
+fn bench_object_same_schema(c: &mut Criterion) {
+ c.bench_function("bench_object_same_schema", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ for _ in 0..25_000 {
+ let mut variant = VariantBuilder::new();
+ let mut object_builder = variant.new_object();
+ object_builder.insert("name", random_string(&mut
rng).as_str());
+ object_builder.insert("age", random::<u32>(&mut rng, 18..100)
as i32);
+ object_builder.insert("likes_cilantro", rng.random_bool(0.5));
+ object_builder.insert("comments", random_long_string(&mut
rng).as_str());
+
+ let mut inner_list_builder = object_builder.new_list("dishes");
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+
+ inner_list_builder.finish();
+ object_builder.finish();
+
+ hint::black_box(variant.finish());
+ }
+ })
+ });
+}
+
+// Creates a list of objects with the same schema (same field names)
+/*
+ {
+ name: String,
+ age: i32,
+ likes_cilantro: bool,
+ comments: Long string
+ dishes: Vec<String>
+ }
+*/
+fn bench_object_list_same_schema(c: &mut Criterion) {
+ c.bench_function("bench_object_list_same_schema", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ let mut variant = VariantBuilder::new();
+
+ let mut list_builder = variant.new_list();
+
+ for _ in 0..25_000 {
Review Comment:
Likewise for the other bencmarks
##########
parquet-variant/benches/builder.rs:
##########
@@ -0,0 +1,389 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+extern crate parquet_variant;
+
+use criterion::*;
+
+use parquet_variant::VariantBuilder;
+use rand::{
+ distr::{uniform::SampleUniform, Alphanumeric},
+ rngs::ThreadRng,
+ Rng,
+};
+use std::{hint, ops::Range};
+
+fn random<T: SampleUniform + PartialEq + PartialOrd>(rng: &mut ThreadRng,
range: Range<T>) -> T {
+ rng.random_range::<T, _>(range)
+}
+
+// generates a string with a 50/50 chance whether it's a short or a long string
+fn random_string(rng: &mut ThreadRng) -> String {
+ let len = rng.random_range::<usize, _>(1..128);
+
+ rng.sample_iter(&Alphanumeric)
+ .take(len)
+ .map(char::from)
+ .collect()
+}
+
+// generates a string guaranteed to be longer than 64 bytes
+fn random_long_string(rng: &mut ThreadRng) -> String {
+ let len = rng.random_range::<usize, _>(65..200);
+
+ rng.sample_iter(&Alphanumeric)
+ .take(len)
+ .map(char::from)
+ .collect()
+}
+
+// Creates an object with field names inserted in reverse lexicographical order
+fn bench_object_field_names_reverse_order(c: &mut Criterion) {
+ c.bench_function("bench_object_field_names_reverse_order", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ let mut variant = VariantBuilder::new();
+ let mut object_builder = variant.new_object();
+
+ for i in 0..50_000 {
+ object_builder.insert(
+ format!("{}", 1000 - i).as_str(),
+ random_string(&mut rng).as_str(),
+ );
+ }
+
+ object_builder.finish();
+ hint::black_box(variant.finish());
+ })
+ });
+}
+
+// Creates objects with a homogenous schema (same field names)
+/*
+ {
+ name: String,
+ age: i32,
+ likes_cilantro: bool,
+ comments: Long string
+ dishes: Vec<String>
+ }
+*/
+fn bench_object_same_schema(c: &mut Criterion) {
+ c.bench_function("bench_object_same_schema", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ for _ in 0..25_000 {
+ let mut variant = VariantBuilder::new();
+ let mut object_builder = variant.new_object();
+ object_builder.insert("name", random_string(&mut
rng).as_str());
+ object_builder.insert("age", random::<u32>(&mut rng, 18..100)
as i32);
+ object_builder.insert("likes_cilantro", rng.random_bool(0.5));
+ object_builder.insert("comments", random_long_string(&mut
rng).as_str());
+
+ let mut inner_list_builder = object_builder.new_list("dishes");
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+ inner_list_builder.append_value(random_string(&mut
rng).as_str());
+
+ inner_list_builder.finish();
+ object_builder.finish();
+
+ hint::black_box(variant.finish());
+ }
+ })
+ });
+}
+
+// Creates a list of objects with the same schema (same field names)
+/*
+ {
+ name: String,
+ age: i32,
+ likes_cilantro: bool,
+ comments: Long string
+ dishes: Vec<String>
+ }
+*/
+fn bench_object_list_same_schema(c: &mut Criterion) {
+ c.bench_function("bench_object_list_same_schema", |b| {
+ b.iter(|| {
+ let mut rng = rand::rng();
+
+ let mut variant = VariantBuilder::new();
+
+ let mut list_builder = variant.new_list();
+
+ for _ in 0..25_000 {
Review Comment:
Here is a profile. There is evidence of a non trivial amount of time spent
in random creation (and I think a bunch of the allocation / free shown is
related to creating `String`s as well)
<img width="1290" alt="Screenshot 2025-06-29 at 5 20 55 AM"
src="https://github.com/user-attachments/assets/5432e896-2b8b-428b-a6bd-6419eeb6102c";
/>
##########
parquet-variant/src/builder.rs:
##########
@@ -861,75 +873,6 @@ mod tests {
assert_eq!(field_ids, vec![1, 2, 0]);
}
- #[test]
- fn test_object_and_metadata_ordering() {
Review Comment:
I think it makes sense to remove this test as it is basically testing the
internal structure / invariants of the builders. I think it makes more sense to
test the public APIs 👍
--
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-unsubscr...@arrow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
