Skip to content

Commit af12254

Browse files
zhuqi-lucaszhuqi-lucas
committed
fix clippy and rebase onto latest upstream/main
- mod.rs: remove unnecessary `#[expect(non_snake_case)]` on `mod struct_` (snake_case-compliant module name, attribute was unfulfilled) - mod.rs: `schema.clone()` -> `Arc::clone(&schema)` per project lint - mod.rs: drop redundant `.to_string()` after `format!` - mod.rs: inline format args in eprintln! - fixed_size_list.rs: change `new(data_type: DataType)` -> `new(data_type: &DataType)` (clippy::needless_pass_by_value); the function only inspects the data_type by reference. Update test callers to pass `&data_type(true)`. Update the dispatcher call site in mod.rs to drop `.clone()` (it already had a reference). - list.rs: elide explicit lifetimes on `list_array` helper (clippy::needless_lifetimes) - list.rs: introduce `NotesRow` and `MatrixRow` type aliases in tests to satisfy clippy::type_complexity - struct_.rs: introduce `OuterRow` type alias in nested_struct_of_struct test, same reason.
1 parent 5bdf350 commit af12254

4 files changed

Lines changed: 74 additions & 68 deletions

File tree

‎datafusion/physical-plan/src/aggregates/group_values/multi_group_by/fixed_size_list.rs‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -48,8 +48,8 @@ pub struct FixedSizeListGroupValueBuilder<T: ArrowPrimitiveType> {
4848
}
4949

5050
impl<T: ArrowPrimitiveType> FixedSizeListGroupValueBuilder<T> {
51-
pub fn new(data_type: DataType) -> Self {
52-
let (field, list_len) = match &data_type {
51+
pub fn new(data_type: &DataType) -> Self {
52+
let (field, list_len) = match data_type {
5353
DataType::FixedSizeList(f, n) => (Arc::clone(f), *n),
5454
other => unreachable!(
5555
"FixedSizeListGroupValueBuilder built with non-FixedSizeList type {other:?}"
@@ -254,7 +254,7 @@ mod tests {
254254
Some([Some(1), Some(2)]),
255255
]);
256256
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
257-
FixedSizeListGroupValueBuilder::new(data_type(true));
257+
FixedSizeListGroupValueBuilder::new(&data_type(true));
258258

259259
for i in 0..input.len() {
260260
builder.append_val(&input, i).unwrap();
@@ -289,7 +289,7 @@ mod tests {
289289
fn equal_to_matches_identical_rows_and_rejects_different() {
290290
let stored = fsl_array(&[Some([Some(1), Some(2)]), Some([Some(3), Some(4)])]);
291291
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
292-
FixedSizeListGroupValueBuilder::new(data_type(true));
292+
FixedSizeListGroupValueBuilder::new(&data_type(true));
293293
builder.append_val(&stored, 0).unwrap();
294294
builder.append_val(&stored, 1).unwrap();
295295

@@ -311,7 +311,7 @@ mod tests {
311311
fn equal_to_handles_outer_nulls() {
312312
let stored = fsl_array(&[None, Some([Some(1), Some(2)])]);
313313
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
314-
FixedSizeListGroupValueBuilder::new(data_type(true));
314+
FixedSizeListGroupValueBuilder::new(&data_type(true));
315315
builder.append_val(&stored, 0).unwrap();
316316
builder.append_val(&stored, 1).unwrap();
317317

@@ -329,7 +329,7 @@ mod tests {
329329
fn equal_to_handles_inner_nulls() {
330330
let stored = fsl_array(&[Some([Some(1), None]), Some([None, None])]);
331331
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
332-
FixedSizeListGroupValueBuilder::new(data_type(true));
332+
FixedSizeListGroupValueBuilder::new(&data_type(true));
333333
builder.append_val(&stored, 0).unwrap();
334334
builder.append_val(&stored, 1).unwrap();
335335

@@ -416,7 +416,7 @@ mod tests {
416416
Some([Some(5), Some(6)]),
417417
]);
418418
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
419-
FixedSizeListGroupValueBuilder::new(data_type(true));
419+
FixedSizeListGroupValueBuilder::new(&data_type(true));
420420
for i in 0..input.len() {
421421
builder.append_val(&input, i).unwrap();
422422
}
@@ -462,7 +462,7 @@ mod tests {
462462
let sliced = full.slice(2, 3);
463463

464464
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
465-
FixedSizeListGroupValueBuilder::new(data_type(true));
465+
FixedSizeListGroupValueBuilder::new(&data_type(true));
466466
for i in 0..sliced.len() {
467467
builder.append_val(&sliced, i).unwrap();
468468
}
@@ -509,7 +509,7 @@ mod tests {
509509
fn take_n_zero_and_full() {
510510
let input = fsl_array(&[Some([Some(1), Some(2)]), Some([Some(3), Some(4)])]);
511511
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
512-
FixedSizeListGroupValueBuilder::new(data_type(true));
512+
FixedSizeListGroupValueBuilder::new(&data_type(true));
513513
for i in 0..input.len() {
514514
builder.append_val(&input, i).unwrap();
515515
}
@@ -538,7 +538,7 @@ mod tests {
538538
let input =
539539
fsl_array(&[Some([Some(1), Some(2)]), None, Some([Some(3), Some(4)])]);
540540
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
541-
FixedSizeListGroupValueBuilder::new(data_type(true));
541+
FixedSizeListGroupValueBuilder::new(&data_type(true));
542542
for i in 0..input.len() {
543543
builder.append_val(&input, i).unwrap();
544544
}
@@ -573,13 +573,13 @@ mod tests {
573573
]);
574574

575575
let mut per_row: FixedSizeListGroupValueBuilder<Int32Type> =
576-
FixedSizeListGroupValueBuilder::new(data_type(true));
576+
FixedSizeListGroupValueBuilder::new(&data_type(true));
577577
for i in 0..input.len() {
578578
per_row.append_val(&input, i).unwrap();
579579
}
580580

581581
let mut vec_b: FixedSizeListGroupValueBuilder<Int32Type> =
582-
FixedSizeListGroupValueBuilder::new(data_type(true));
582+
FixedSizeListGroupValueBuilder::new(&data_type(true));
583583
vec_b.vectorized_append(&input, &[0, 1, 2, 3]).unwrap();
584584
assert_eq!(vec_b.len(), per_row.len());
585585

@@ -611,7 +611,7 @@ mod tests {
611611
fn size_grows_with_appends() {
612612
let input = fsl_array(&[Some([Some(1), Some(2)])]);
613613
let mut builder: FixedSizeListGroupValueBuilder<Int32Type> =
614-
FixedSizeListGroupValueBuilder::new(data_type(true));
614+
FixedSizeListGroupValueBuilder::new(&data_type(true));
615615
let s0 = builder.size();
616616
for _ in 0..16 {
617617
builder.append_val(&input, 0).unwrap();
@@ -623,7 +623,7 @@ mod tests {
623623
#[test]
624624
fn build_empty_builder_returns_empty_array() {
625625
let builder: FixedSizeListGroupValueBuilder<Int32Type> =
626-
FixedSizeListGroupValueBuilder::new(data_type(true));
626+
FixedSizeListGroupValueBuilder::new(&data_type(true));
627627
let out = Box::new(builder).build();
628628
let out = out.as_any().downcast_ref::<FixedSizeListArray>().unwrap();
629629
assert_eq!(out.len(), 0);

‎datafusion/physical-plan/src/aggregates/group_values/multi_group_by/list.rs‎

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ impl<O: OffsetSizeTrait> ListGroupValueBuilder<O> {
5959
}
6060
}
6161

62-
fn list_array<'a>(array: &'a ArrayRef) -> &'a GenericListArray<O> {
62+
fn list_array(array: &ArrayRef) -> &GenericListArray<O> {
6363
array
6464
.as_any()
6565
.downcast_ref::<GenericListArray<O>>()
@@ -412,11 +412,12 @@ mod tests {
412412

413413
// Build LargeList<Struct<id: Utf8, n: Int32>> arrays.
414414
//
415-
// notes_v(rows) where each row is Option<Vec<(Option<&str>, Option<i32>)>>:
415+
// NotesRow models one outer LargeList row of Struct<id, n>:
416416
// None -> null outer list
417417
// Some(vec![]) -> empty list
418418
// Some(vec![(id, n), ...]) -> list with struct entries
419-
let notes_v = |rows: &[Option<Vec<(Option<&str>, Option<i32>)>>]| -> ArrayRef {
419+
type NotesRow<'a> = Option<Vec<(Option<&'a str>, Option<i32>)>>;
420+
let notes_v = |rows: &[NotesRow<'_>]| -> ArrayRef {
420421
let struct_builder = StructBuilder::new(
421422
struct_fields.clone(),
422423
vec![
@@ -768,9 +769,12 @@ mod tests {
768769
true,
769770
)]));
770771

771-
// Helper: produce a List<List<Int32>> from
772-
// Option<Vec<Option<Vec<Option<i32>>>>>
773-
let mk = |rows: &[Option<Vec<Option<Vec<Option<i32>>>>>]| -> ArrayRef {
772+
// MatrixRow models one outer List<List<Int32>> row:
773+
// None -> null outer list
774+
// Some(vec![...]) -> list of sublists, each sublist itself
775+
// Option<Vec<Option<i32>>>
776+
type MatrixRow = Option<Vec<Option<Vec<Option<i32>>>>>;
777+
let mk = |rows: &[MatrixRow]| -> ArrayRef {
774778
let inner = ListBuilder::new(Int32Builder::new())
775779
.with_field(Arc::clone(&inner_field));
776780
let mut outer = ListBuilder::new(inner).with_field(Arc::clone(&outer_field));

‎datafusion/physical-plan/src/aggregates/group_values/multi_group_by/mod.rs‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ pub mod bytes_view;
2323
mod fixed_size_list;
2424
mod list;
2525
pub mod primitive;
26-
#[allow(non_snake_case)]
2726
mod struct_;
2827

2928
use std::mem::{self, size_of};
@@ -1002,7 +1001,7 @@ fn make_group_column(field: &Field) -> Result<Box<dyn GroupColumn>> {
10021001
macro_rules! instantiate_fsl {
10031002
($t:ty) => {{
10041003
let b = fixed_size_list::FixedSizeListGroupValueBuilder::<$t>::new(
1005-
data_type.clone(),
1004+
data_type,
10061005
);
10071006
v.push(Box::new(b) as _);
10081007
}};
@@ -1613,7 +1612,8 @@ mod tests {
16131612
) {
16141613
use super::super::row::GroupValuesRows;
16151614

1616-
let mut col_gv = GroupValuesColumn::<false>::try_new(schema.clone()).unwrap();
1615+
let mut col_gv =
1616+
GroupValuesColumn::<false>::try_new(Arc::clone(&schema)).unwrap();
16171617
let mut groups = Vec::new();
16181618
col_gv.intern(cols, &mut groups).unwrap();
16191619
let col_size = col_gv.size();
@@ -1632,8 +1632,7 @@ mod tests {
16321632
col_size={col_size}, row_size={row_size}, ratio={ratio:.2}x",
16331633
);
16341634
eprintln!(
1635-
"{label}: col_size={col_size} B, row_size={row_size} B, savings={:.1}x",
1636-
ratio,
1635+
"{label}: col_size={col_size} B, row_size={row_size} B, savings={ratio:.1}x"
16371636
);
16381637
}
16391638

@@ -1716,9 +1715,9 @@ mod tests {
17161715
s.field_builder::<StringBuilder>(0)
17171716
.unwrap()
17181717
.append_value(format!("id-{i}-bbbbbbbbbbbbbbbb"));
1719-
s.field_builder::<StringBuilder>(1).unwrap().append_value(
1720-
format!("second description for entry {i}.....").to_string(),
1721-
);
1718+
s.field_builder::<StringBuilder>(1)
1719+
.unwrap()
1720+
.append_value(format!("second description for entry {i}....."));
17221721
s.append(true);
17231722
list_b.append(true);
17241723
}

‎datafusion/physical-plan/src/aggregates/group_values/multi_group_by/struct_.rs‎

Lines changed: 43 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -477,48 +477,51 @@ mod tests {
477477

478478
// Build a Struct<Struct<id: Utf8, n: Int32>, tag: Int32> array with
479479
// four rows: two identical, one distinct, one null outer.
480-
let mk_array =
481-
|rows: &[Option<((Option<&str>, Option<i32>), Option<i32>)>]| -> ArrayRef {
482-
// Inner struct children
483-
let mut inner_ids: Vec<Option<&str>> = Vec::with_capacity(rows.len());
484-
let mut inner_ns: Vec<Option<i32>> = Vec::with_capacity(rows.len());
485-
let mut tags: Vec<Option<i32>> = Vec::with_capacity(rows.len());
486-
let mut outer_validity: Vec<bool> = Vec::with_capacity(rows.len());
487-
let mut inner_validity: Vec<bool> = Vec::with_capacity(rows.len());
488-
for row in rows {
489-
match row {
490-
None => {
491-
inner_ids.push(None);
492-
inner_ns.push(None);
493-
tags.push(None);
494-
outer_validity.push(false);
495-
inner_validity.push(false);
496-
}
497-
Some(((id, n), tag)) => {
498-
inner_ids.push(*id);
499-
inner_ns.push(*n);
500-
tags.push(*tag);
501-
outer_validity.push(true);
502-
inner_validity.push(true);
503-
}
480+
//
481+
// OuterRow models one outer struct row: outer null is `None`,
482+
// otherwise `Some((inner, tag))` where `inner = (id, n)`.
483+
type OuterRow<'a> = Option<((Option<&'a str>, Option<i32>), Option<i32>)>;
484+
let mk_array = |rows: &[OuterRow<'_>]| -> ArrayRef {
485+
// Inner struct children
486+
let mut inner_ids: Vec<Option<&str>> = Vec::with_capacity(rows.len());
487+
let mut inner_ns: Vec<Option<i32>> = Vec::with_capacity(rows.len());
488+
let mut tags: Vec<Option<i32>> = Vec::with_capacity(rows.len());
489+
let mut outer_validity: Vec<bool> = Vec::with_capacity(rows.len());
490+
let mut inner_validity: Vec<bool> = Vec::with_capacity(rows.len());
491+
for row in rows {
492+
match row {
493+
None => {
494+
inner_ids.push(None);
495+
inner_ns.push(None);
496+
tags.push(None);
497+
outer_validity.push(false);
498+
inner_validity.push(false);
499+
}
500+
Some(((id, n), tag)) => {
501+
inner_ids.push(*id);
502+
inner_ns.push(*n);
503+
tags.push(*tag);
504+
outer_validity.push(true);
505+
inner_validity.push(true);
504506
}
505507
}
506-
let inner_id_arr: ArrayRef = Arc::new(StringArray::from(inner_ids));
507-
let inner_n_arr: ArrayRef = Arc::new(Int32Array::from(inner_ns));
508-
let inner_null = arrow::buffer::NullBuffer::from(inner_validity);
509-
let inner = Arc::new(StructArray::new(
510-
inner_fields.clone(),
511-
vec![inner_id_arr, inner_n_arr],
512-
Some(inner_null),
513-
)) as ArrayRef;
514-
let tag_arr: ArrayRef = Arc::new(Int32Array::from(tags));
515-
let outer_null = arrow::buffer::NullBuffer::from(outer_validity);
516-
Arc::new(StructArray::new(
517-
outer_fields.clone(),
518-
vec![inner, tag_arr],
519-
Some(outer_null),
520-
))
521-
};
508+
}
509+
let inner_id_arr: ArrayRef = Arc::new(StringArray::from(inner_ids));
510+
let inner_n_arr: ArrayRef = Arc::new(Int32Array::from(inner_ns));
511+
let inner_null = arrow::buffer::NullBuffer::from(inner_validity);
512+
let inner = Arc::new(StructArray::new(
513+
inner_fields.clone(),
514+
vec![inner_id_arr, inner_n_arr],
515+
Some(inner_null),
516+
)) as ArrayRef;
517+
let tag_arr: ArrayRef = Arc::new(Int32Array::from(tags));
518+
let outer_null = arrow::buffer::NullBuffer::from(outer_validity);
519+
Arc::new(StructArray::new(
520+
outer_fields.clone(),
521+
vec![inner, tag_arr],
522+
Some(outer_null),
523+
))
524+
};
522525

523526
let mut gv = new_group_values(schema, &GroupOrdering::None).unwrap();
524527
let batch = mk_array(&[

0 commit comments

Comments
 (0)