alamb commented on code in PR #9372: URL: https://github.com/apache/arrow-rs/pull/9372#discussion_r3736054984
########## parquet/tests/arrow_reader/alp.rs: ########## @@ -0,0 +1,258 @@ +// 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. + +use arrow::compute::concat_batches; +use arrow::util::test_util::parquet_test_data; +use arrow_array::cast::as_primitive_array; +use arrow_array::types::{Float32Type, Float64Type}; +use arrow_array::{Array, Float32Array, Float64Array, RecordBatch}; +use arrow_csv::ReaderBuilder as CsvReaderBuilder; +use arrow_schema::{DataType, Field, Schema}; +use bytes::Bytes; +use parquet::arrow::ArrowWriter; +use parquet::arrow::arrow_reader::ArrowReaderBuilder; +use parquet::basic::Encoding; +use parquet::file::properties::{WriterProperties, WriterVersion}; +use std::fs::File; +use std::path::PathBuf; +use std::sync::Arc; + +#[test] +fn test_read_f32_alp() { + let data_dir = PathBuf::from(parquet_test_data()); + let parquet_path = data_dir.join("alp_float_arade.parquet"); + let expected_csv_path = data_dir.join("alp_arade_expect.csv"); + + let expected = read_expected_csv_batch(&expected_csv_path); + let actual = read_parquet_batch(&parquet_path); + + assert_eq!(actual.schema(), expected.schema(), "schema mismatch"); + assert_eq!( + actual.num_columns(), + expected.num_columns(), + "column mismatch" + ); + assert_eq!(actual.num_rows(), expected.num_rows(), "row count mismatch"); + + for col_idx in 0..actual.num_columns() { + let col_name = actual.schema().field(col_idx).name().clone(); + let actual_col = as_primitive_array::<Float32Type>(actual.column(col_idx).as_ref()); + let expected_col = as_primitive_array::<Float32Type>(expected.column(col_idx).as_ref()); + + for row_idx in 0..actual.num_rows() { + assert_eq!( + actual_col.is_valid(row_idx), + expected_col.is_valid(row_idx), + "null mismatch at column {col_name} row {row_idx}" + ); + if actual_col.is_valid(row_idx) { + let actual_value = actual_col.value(row_idx); + let expected_value = expected_col.value(row_idx); + assert!( + actual_value.to_bits() == expected_value.to_bits(), + "bit mismatch at column {col_name} row {row_idx}: expected={expected_value} actual={actual_value}" + ); + } + } + } +} + +/// Write the arade values with the ALP encoder and read them back, over real +/// float data rather than synthetic decimals. +/// +/// This checks losslessness only, not compression: these are `f32` values whose +/// encoded integers need 31 bits and which except 6.4% of the time, so ALP is +/// larger than PLAIN here, a property of the data rather than the encoder. +/// ALP's win on arade in the paper is on the `f64` version of the dataset. +#[test] +fn test_write_f32_alp_roundtrip() { Review Comment: done ########## parquet/src/encodings/encoding/mod.rs: ########## @@ -84,26 +86,90 @@ pub fn get_encoder<T: DataType>( encoding: Encoding, descr: &ColumnDescPtr, ) -> Result<Box<dyn Encoder<T>>> { - let encoder: Box<dyn Encoder<T>> = match encoding { - Encoding::PLAIN => Box::new(PlainEncoder::new()), - Encoding::RLE_DICTIONARY | Encoding::PLAIN_DICTIONARY => { - return Err(general_err!( - "Cannot initialize this encoding through this function" - )); + <T::T as private::GetEncoder>::get_encoder(descr, encoding) +} + +pub(crate) mod private { + use super::*; + + /// A trait that allows getting an [`Encoder`] implementation for a [`DataType`] + /// with the corresponding [`ParquetValueType`]. This is necessary to support + /// [`Encoder`] implementations that may not be applicable for all [`DataType`] + /// and by extension all [`ParquetValueType`], such as ALP, which encodes only + /// floating-point columns. + /// + /// [`ParquetValueType`]: crate::data_type::private::ParquetValueType + pub trait GetEncoder { Review Comment: I asked claude about this -- it says this trait is needed because Since get_encoder<T: DataType> is generic, every arm of its match must type-check for every T it's monomorphized with. So this doesn't compile: ```rust Encoding::ALP => match T::get_physical_type() { Type::FLOAT | Type::DOUBLE => Box::new(AlpEncoder::new()), // error: `i32: AlpFloat` not satisfied ... } ``` Because we basically need to only invoke this for certain T types (f32 and f64) -- 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]
