laskoviymishka commented on code in PR #3107:
URL: https://github.com/apache/iceberg-rust/pull/3107#discussion_r4131000900
##########
crates/iceberg/src/scan/mod.rs:
##########
@@ -652,1178 +652,40 @@ pub(crate) struct BoundPredicates {
}
#[cfg(test)]
-pub mod tests {
+mod tests {
Review Comment:
Now that `mod tests` is private and the only previously-`pub` items moved
out to `test_utils::scan`, the `#![allow(missing_docs)]` just below doesn't
suppress anything anymore — worth dropping while we're in here.
##########
crates/iceberg/src/test_utils/mod.rs:
##########
@@ -0,0 +1,39 @@
+// 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.
+
+//! Test utilities.
+//!
+//! Compiled under `cfg(test)`, or behind the `test-utils` feature for other
+//! crates in this workspace that need these fixtures from their own tests.
Review Comment:
Small thing: the feature only ever exposes `check_record_batches` and
`test_runtime` to other crates — `scan`, `encryption`, and `delete_vector` stay
`#[cfg(test)]` + `pub(crate)`, so they're unreachable cross-crate even with
`test-utils` on. I'd tighten the wording so a future contributor doesn't flip
the feature on expecting `TableTestFixture` and hit a compile error.
##########
crates/iceberg/Cargo.toml:
##########
@@ -31,6 +31,9 @@ repository = { workspace = true }
[features]
default = []
+# Exposes `iceberg::test_utils` for use by other crates' tests. Not public API;
+# enable it from `[dev-dependencies]` only.
+test-utils = ["dep:expect-test"]
Review Comment:
`test-utils` exposes exactly two items cross-crate — `check_record_batches`
and `test_runtime` — and neither touches `iceberg` internals; they're built
purely on already-public API. Those could live in the existing
`iceberg_test_utils` crate (already wired as a dev-dependency into `rest`)
instead, which would let us drop the new feature and the optional `expect-test`
dep from the published crate entirely, for the sake of a single consumer.
The crate-internal fixtures (`scan`, `encryption`, `delete_vector`) do need
`pub(crate)` access and are correctly `#[cfg(test)]`-only — that part's right.
It's just the feature I'd weigh against relocating those two helpers, so we
don't end up maintaining both a `test-utils` feature and an
`iceberg_test_utils` crate for overlapping jobs. Not a blocker — wdyt?
##########
crates/iceberg/src/test_utils/scan.rs:
##########
@@ -0,0 +1,1160 @@
+// 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.
+
+//! Shared test fixtures for the table scan API.
+
+use std::collections::HashMap;
+use std::fs;
+use std::fs::File;
+use std::sync::Arc;
+
+use arrow_array::cast::AsArray;
+use arrow_array::{
+ Array, ArrayRef, BooleanArray, Float64Array, Int32Array, Int64Array,
RecordBatch, StringArray,
+};
+use minijinja::value::Value;
+use minijinja::{AutoEscape, Environment, context};
+use parquet::arrow::{ArrowWriter, PARQUET_FIELD_ID_META_KEY};
+use parquet::basic::Compression;
+use parquet::file::properties::WriterProperties;
+use tempfile::TempDir;
+use uuid::Uuid;
+
+use crate::TableIdent;
+use crate::io::{FileIO, OutputFile};
+use crate::metadata_columns::{
+ RESERVED_COL_NAME_DELETE_FILE_PATH, RESERVED_COL_NAME_DELETE_FILE_POS,
+ RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER,
RESERVED_FIELD_ID_DELETE_FILE_PATH,
+ RESERVED_FIELD_ID_DELETE_FILE_POS,
+};
+use crate::spec::{
+ DataContentType, DataFileBuilder, DataFileFormat, FormatVersion, Literal,
ManifestEntry,
+ ManifestListWriter, ManifestStatus, ManifestWriterBuilder, PartitionSpec,
Struct, StructType,
+ TableMetadata, TableMetadataBuilder,
+};
+use crate::table::Table;
+use crate::test_utils::test_runtime;
+
+fn render_template(template: &str, ctx: Value) -> String {
+ let mut env = Environment::new();
+ env.set_auto_escape_callback(|_| AutoEscape::None);
+ env.render_str(template, ctx).unwrap()
+}
+
+/// Asserts every row of the `_last_updated_sequence_number` column across all
+/// batches equals `expected` (or is null when `expected` is `None`), decoding
+/// the logical value independent of the physical (run-end) encoding.
+pub fn assert_last_updated_seq_all(batches: &[RecordBatch], expected:
Option<i64>) {
+ use arrow_cast::cast;
+ use arrow_schema::DataType;
+ for batch in batches {
+ let col = batch
+ .column_by_name(RESERVED_COL_NAME_LAST_UPDATED_SEQUENCE_NUMBER)
+ .expect("_last_updated_sequence_number column should be present");
+ let logical = cast(col, &DataType::Int64).unwrap();
+ let values = logical.as_primitive::<arrow_array::types::Int64Type>();
+ for i in 0..values.len() {
+ let actual = (!values.is_null(i)).then(|| values.value(i));
+ assert_eq!(actual, expected, "row {i}");
+ }
+ }
+}
+
+pub struct TableTestFixture {
Review Comment:
These are all `pub` even though the module is declared `#[cfg(test)]
pub(crate) mod scan;`, so nothing actually leaks today. But it's out of step
with `encryption.rs`/`delete_vector.rs` two files over, which mark their
fixtures `pub(crate)` — and if the `#[cfg(test)]` here ever gets relaxed to
match the rest of the module, these ~15 functions would silently land in the
real public API. I'd mark the struct, its fields, and the methods `pub(crate)`
to match the siblings.
--
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]
