alamb commented on code in PR #24366:
URL: https://github.com/apache/datafusion/pull/24366#discussion_r3785148999
##########
datafusion/catalog-listing/src/helpers.rs:
##########
@@ -30,7 +30,7 @@ use datafusion_expr::{BinaryExpr, Operator, lit, utils};
use arrow::{
array::AsArray,
- datatypes::{DataType, Field},
+ datatypes::{DataType, Field, Metadata},
Review Comment:
Arrow 60 introduces a new metadata type:
- https://github.com/apache/arrow-rs/pull/10075
##########
Cargo.toml:
##########
@@ -176,7 +176,7 @@ liblzma = { version = "0.4.6", features = ["static"] }
log = "^0.4"
memchr = "2.8.1"
num-traits = { version = "0.2" }
-object_store = { version = "0.13.2", default-features = false }
+object_store = { version = "0.14.1", default-features = false }
Review Comment:
`parquet` on arrow-rs main now requires `object_store` 0.14:
- https://github.com/apache/arrow-rs/pull/10375
##########
datafusion/proto-common/src/from_proto/mod.rs:
##########
@@ -28,7 +28,7 @@ use arrow::datatypes::{
TimeUnit, UnionFields, UnionMode, i256,
};
use arrow::ipc::{
- convert::fb_to_schema,
+ convert::try_fb_to_schema,
Review Comment:
`fb_to_schema` is deprecated in favor of the fallible `try_fb_to_schema`
(same for the `datasource-arrow` changes):
- https://github.com/apache/arrow-rs/pull/10647
##########
datafusion/functions/src/core/arrow_field.rs:
##########
@@ -125,8 +126,18 @@ impl ScalarUDFImpl for ArrowFieldFunc {
// Build the metadata map array (same pattern as arrow_metadata.rs)
let metadata = field.metadata();
- let mut map_builder =
- MapBuilder::new(None, StringBuilder::new(), StringBuilder::new());
+ // Match the field names declared in `return_type` (the arrow-rs
+ // default changed to `key`/`value` in arrow 60)
+ let map_field_names = MapFieldNames {
Review Comment:
Arrow 60 changed the default `MapBuilder` field names from `keys`/`values`
to the spec-canonical `key`/`value`, so the built array no longer matched the
declared return type:
- https://github.com/apache/arrow-rs/pull/10517
##########
datafusion/functions-aggregate/benches/array_agg.rs:
##########
@@ -65,8 +64,7 @@ pub fn create_list_array<T>(
zero_length_lists_probability: f32,
) -> ListArray
where
- T: ArrowPrimitiveType,
- StandardUniform: Distribution<T::Native>,
+ T: ArrowPrimitiveType<Native = i64>,
Review Comment:
Arrow 60 upgraded to `rand` 0.10, so `bench_util`'s `StandardUniform` bounds
can no longer be stated from DataFusion's `rand` 0.9:
- https://github.com/apache/arrow-rs/pull/10566
- Filed https://github.com/apache/arrow-rs/issues/10685 upstream
##########
datafusion/physical-plan/src/spill/spill_manager.rs:
##########
@@ -220,35 +219,14 @@ impl GetSlicedSize for RecordBatch {
let mut total = 0;
for array in self.columns() {
let data = array.to_data();
+ // Since https://github.com/apache/arrow-rs/issues/8230 this also
+ // accounts for the variadic data buffers retained by view arrays
Review Comment:
`ArrayData::get_slice_memory_size` now accounts for the variadic data
buffers retained by view arrays, so the workaround here double-counted:
- https://github.com/apache/arrow-rs/pull/10519
##########
datafusion-cli/src/main.rs:
##########
@@ -642,9 +642,9 @@ mod tests {
+-----------------------------------+-----------------+---------------------+------+------------------+
| filename | file_size_bytes |
metadata_size_bytes | hits | extra |
+-----------------------------------+-----------------+---------------------+------+------------------+
- | alltypes_plain.parquet | 1851 | 8794
| 1 | page_index=false |
- | alltypes_tiny_pages.parquet | 454233 | 268970
| 2 | page_index=true |
- | lz4_raw_compressed_larger.parquet | 380836 | 1331
| 1 | page_index=false |
+ | alltypes_plain.parquet | 1851 | 8970
| 1 | page_index=false |
Review Comment:
Parquet metadata heap sizes shifted slightly:
- https://github.com/apache/arrow-rs/pull/9619 (added
`nan_count`/`nan_counts` to statistics and the page index — the largest shift
is in `alltypes_tiny_pages`, the only file with `page_index=true`)
- https://github.com/apache/arrow-rs/pull/10149 (widened row group ordinals
from `i16` to `i32`)
##########
datafusion/functions-aggregate/src/array_agg.rs:
##########
@@ -1760,7 +1760,7 @@ mod tests {
acc2.update_batch(&[string_list_data([vec!["e", "f", "g"]])])?;
acc1 = merge(acc1, acc2)?;
- assert_eq!(acc1.size(), 2274);
+ assert_eq!(acc1.size(), 2194);
Review Comment:
`Field` is 80 bytes smaller with the new `Metadata` representation:
- https://github.com/apache/arrow-rs/pull/10075
##########
datafusion/core/src/datasource/file_format/csv.rs:
##########
@@ -136,6 +136,7 @@ mod tests {
},
range: Default::default(),
attributes: Attributes::default(),
+ extensions: Default::default(),
Review Comment:
`object_store` 0.14 added an `extensions` field to `GetResult`/`ListResult`:
- https://github.com/apache/arrow-rs-object-store/pull/743
##########
Cargo.toml:
##########
@@ -103,7 +103,7 @@ arrow-avro = { version = "59.2.0", default-features =
false, features = [
arrow-buffer = { version = "59.2.0", default-features = false }
arrow-data = { version = "59.2.0", default-features = false }
arrow-flight = { version = "59.2.0", features = [
- "flight-sql-experimental",
+ "flight-sql",
Review Comment:
The `flight-sql-experimental` feature was renamed to `flight-sql` and the
old alias has now been removed:
- https://github.com/apache/arrow-rs/pull/7546
- https://github.com/apache/arrow-rs/pull/10335
##########
datafusion/sqllogictest/test_files/datetime/arith_interval_double.slt:
##########
@@ -33,9 +33,13 @@ SELECT arrow_typeof(interval '1 second' * 900)
# Divide an interval by a scalar
# interval '1 hour' / 1.5 → 00:40:00
-query error Invalid interval arithmetic operation: Interval\(MonthDayNano\) /
Interval\(MonthDayNano\)
+query ?
SELECT interval '1 hour' / 1.5
+----
+40 mins
Review Comment:
Arrow now supports interval multiplication/division by numeric types:
- https://github.com/apache/arrow-rs/pull/10336
- https://github.com/apache/arrow-rs/pull/10409
##########
datafusion/sqllogictest/test_files/push_down_filter_parquet.slt:
##########
@@ -268,7 +268,7 @@ EXPLAIN ANALYZE SELECT * FROM topk_single_col ORDER BY b
DESC LIMIT 1;
----
Plan with Metrics
01)SortExec: TopK(fetch=1), expr=[b@1 DESC], preserve_partitioning=[false],
filter=[b@1 IS NULL OR b@1 > bd], metrics=[output_rows=1, output_batches=1,
row_replacements=1]
-02)--DataSourceExec: file_groups={1 group:
[[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/push_down_filter_parquet/topk_single_col.parquet]]},
projection=[a, b, c], file_type=parquet, predicate=DynamicFilter [ b@1 IS NULL
OR b@1 > bd ], sort_order_for_reorder=[b@1 DESC], reverse_row_groups=true,
dynamic_rg_pruning=eligible, pruning_predicate=b_null_count@0 > 0 OR
b_null_count@0 != row_count@2 AND b_max@1 > bd, required_guarantees=[],
metrics=[output_rows=4, output_batches=1, files_ranges_pruned_statistics=1
total → 1 matched, row_groups_pruned_statistics=1 total → 1 matched,
row_groups_pruned_bloom_filter=1 total → 1 matched, page_index_pages_pruned=0
total → 0 matched, page_index_rows_pruned=0 total → 0 matched,
limit_pruned_row_groups=0 total → 0 matched, batches_split=0,
file_open_errors=0, file_scan_errors=0, files_opened=1, files_processed=1,
num_predicate_creation_errors=0, predicate_evaluation_errors=0,
pushdown_rows_matched=4, pushdown_rows_pruned=0,
predicate_cache_inner_records=4, predicate_cache_records=4,
scan_efficiency_ratio=21.62% (222/1.03 K)]
+02)--DataSourceExec: file_groups={1 group:
[[WORKSPACE_ROOT/datafusion/sqllogictest/test_files/scratch/push_down_filter_parquet/topk_single_col.parquet]]},
projection=[a, b, c], file_type=parquet, predicate=DynamicFilter [ b@1 IS NULL
OR b@1 > bd ], sort_order_for_reorder=[b@1 DESC], reverse_row_groups=true,
dynamic_rg_pruning=eligible, pruning_predicate=b_null_count@0 > 0 OR
b_null_count@0 != row_count@2 AND b_max@1 > bd, required_guarantees=[],
metrics=[output_rows=4, output_batches=1, files_ranges_pruned_statistics=1
total → 1 matched, row_groups_pruned_statistics=1 total → 1 matched,
row_groups_pruned_bloom_filter=1 total → 1 matched, page_index_pages_pruned=0
total → 0 matched, page_index_rows_pruned=0 total → 0 matched,
limit_pruned_row_groups=0 total → 0 matched, batches_split=0,
file_open_errors=0, file_scan_errors=0, files_opened=1, files_processed=1,
num_predicate_creation_errors=0, predicate_evaluation_errors=0,
pushdown_rows_matched=4, pushdown_rows_pruned=0,
predicate_cache_inner_records=4, predicate_cache_records=4,
scan_efficiency_ratio=21.94% (222/1.01 K)]
Review Comment:
The `scan_efficiency_ratio` denominators shifted because the parquet files
written by this test got ~15 bytes smaller: since
https://github.com/apache/arrow-rs/pull/9619 float/double statistics use IEEE
754 total order, which is not backwards compatible with old readers' signed
comparison, so the writer no longer duplicates min/max into the deprecated
`Statistics` `min`/`max` fields for floating point columns.
Verified by writing the same table with arrow 59.2 vs the pinned rev: 1027
bytes (footer thrift 686) vs 1012 bytes (footer thrift 668).
##########
datafusion/core/tests/user_defined/user_defined_aggregates.rs:
##########
@@ -1018,11 +1018,8 @@ async fn test_metadata_based_aggregate() -> Result<()> {
let data_array = Arc::new(UInt64Array::from(vec![0, 5, 10, 15, 20])) as
ArrayRef;
let schema = Arc::new(Schema::new(vec![
Field::new("no_metadata", DataType::UInt64, true),
- Field::new("with_metadata", DataType::UInt64, true).with_metadata(
- [("modify_values".to_string(), "double_output".to_string())]
- .into_iter()
- .collect(),
- ),
+ Field::new("with_metadata", DataType::UInt64, true)
Review Comment:
the new Metadata api is nicer to work with I think 😍
##########
datafusion/common/src/heap_size.rs:
##########
@@ -396,6 +396,19 @@ impl DFHeapSize for UnionFields {
}
}
+impl DFHeapSize for Metadata {
+ fn heap_size(&self, ctx: &mut DFHeapSizeCtx) -> usize {
Review Comment:
Could be simpler with nicer upstream API. Filed a ticket to track:
- https://github.com/apache/arrow-rs/issues/10684
##########
datafusion-cli/src/main.rs:
##########
@@ -842,14 +842,14 @@ mod tests {
])?
.sort(vec![col("filename").sort(true, false)])?;
let rbs = df.collect().await?;
- assert_snapshot!(batches_to_string(&rbs),@r"
+ assert_snapshot!(batches_to_string(&rbs),@r#"
+---------------------+-----------+-----------------+------+------+
| metadata_size_bytes | filename | file_size_bytes | etag | hits |
+---------------------+-----------+-----------------+------+------+
- | 212 | 0.parquet | 3642 | 0 | 2 |
- | 212 | 1.parquet | 3642 | 1 | 2 |
+ | 216 | 0.parquet | 3620 | "0" | 2 |
Review Comment:
"0" due to
- apache/arrow-rs-object-store#770
##########
datafusion/expr/src/expr.rs:
##########
@@ -658,7 +658,12 @@ pub fn intersect_metadata_for_union<'a>(
}
match &mut intersected {
None => {
- intersected = Some(metadata.clone());
+ intersected = Some(
Review Comment:
This should be nicer with an API like
- https://github.com/apache/arrow-rs/issues/10683
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
