Copilot commented on code in PR #3485:
URL: https://github.com/apache/iceberg-rust/pull/3485#discussion_r4538767878
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -3075,4 +3079,88 @@ message schema {
assert_eq!(fallback.len(), 1);
assert_top_list(&fallback[0]);
}
+
+ /// Reads a file that stores `date` columns through a schema that promotes
them to
+ /// `timestamp` and `timestamp_ns`, with an unpromoted column projected
alongside them.
+ #[tokio::test]
+ async fn test_read_date_promoted_to_timestamp_with_other_columns() {
+ use arrow_array::{
+ Date32Array, Int32Array, TimestampMicrosecondArray,
TimestampNanosecondArray,
+ };
+
+ use crate::arrow::schema_to_arrow_schema;
+
+ fn schema(d: PrimitiveType, e: PrimitiveType) -> Schema {
+ Schema::builder()
+ .with_fields(vec![
+ NestedField::optional(1, "d", Type::Primitive(d)).into(),
+ NestedField::required(2, "e", Type::Primitive(e)).into(),
+ NestedField::optional(3, "a",
Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap()
+ }
+
+ let days = [18000, -1, 0];
+ let ints = Arc::new(Int32Array::from(vec![1, 2, 3])) as ArrayRef;
+ let old_schema = schema(PrimitiveType::Date, PrimitiveType::Date);
+ let old_batch = RecordBatch::try_new(
+ Arc::new(schema_to_arrow_schema(&old_schema).unwrap()),
+ vec![
+ Arc::new(Date32Array::from(days.to_vec())),
+ Arc::new(Date32Array::from(days.to_vec())),
+ ints.clone(),
+ ],
+ )
+ .unwrap();
+ let new_schema = Arc::new(schema(PrimitiveType::Timestamp,
PrimitiveType::TimestampNs));
+ let micros: Vec<i64> = days
+ .iter()
+ .map(|d| i64::from(*d) * 86_400_000_000)
+ .collect();
+ let expected = RecordBatch::try_new(
+ Arc::new(schema_to_arrow_schema(&new_schema).unwrap()),
+ vec![
+ Arc::new(TimestampMicrosecondArray::from(micros.clone())),
+ Arc::new(TimestampNanosecondArray::from(
+ micros.iter().map(|us| us * 1000).collect::<Vec<_>>(),
+ )),
+ ints,
+ ],
+ )
+ .unwrap();
+
+ let tmp_dir = TempDir::new().unwrap();
+ let file_path = format!("{}/date.parquet",
tmp_dir.path().to_str().unwrap());
+ let mut writer =
+ ArrowWriter::try_new(File::create(&file_path).unwrap(),
old_batch.schema(), None)
+ .unwrap();
+ writer.write(&old_batch).unwrap();
+ writer.close().unwrap();
+
+ let reader = ArrowReaderBuilder::new(FileIO::new_with_fs(),
Runtime::current()).build();
+ let tasks = Box::pin(futures::stream::iter(
+ vec![Ok(FileScanTask::builder()
+
.with_file_size_in_bytes(std::fs::metadata(&file_path).unwrap().len())
+ .with_start(0)
+ .with_length(0)
Review Comment:
`FileScanTask` is being built with `.with_length(0)`. If `length` is
interpreted as the byte-range length to read, this can produce an empty scan
(or rely on undocumented fallback behavior). Prefer setting `length` to the
file size (or omitting `start/length` if the builder supports full-file reads)
so the test is robust to implementation changes.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -3075,4 +3079,88 @@ message schema {
assert_eq!(fallback.len(), 1);
assert_top_list(&fallback[0]);
}
+
+ /// Reads a file that stores `date` columns through a schema that promotes
them to
+ /// `timestamp` and `timestamp_ns`, with an unpromoted column projected
alongside them.
+ #[tokio::test]
+ async fn test_read_date_promoted_to_timestamp_with_other_columns() {
+ use arrow_array::{
+ Date32Array, Int32Array, TimestampMicrosecondArray,
TimestampNanosecondArray,
+ };
+
+ use crate::arrow::schema_to_arrow_schema;
+
+ fn schema(d: PrimitiveType, e: PrimitiveType) -> Schema {
+ Schema::builder()
+ .with_fields(vec![
+ NestedField::optional(1, "d", Type::Primitive(d)).into(),
+ NestedField::required(2, "e", Type::Primitive(e)).into(),
+ NestedField::optional(3, "a",
Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap()
+ }
+
+ let days = [18000, -1, 0];
+ let ints = Arc::new(Int32Array::from(vec![1, 2, 3])) as ArrayRef;
+ let old_schema = schema(PrimitiveType::Date, PrimitiveType::Date);
+ let old_batch = RecordBatch::try_new(
+ Arc::new(schema_to_arrow_schema(&old_schema).unwrap()),
+ vec![
+ Arc::new(Date32Array::from(days.to_vec())),
+ Arc::new(Date32Array::from(days.to_vec())),
+ ints.clone(),
+ ],
+ )
+ .unwrap();
+ let new_schema = Arc::new(schema(PrimitiveType::Timestamp,
PrimitiveType::TimestampNs));
+ let micros: Vec<i64> = days
+ .iter()
+ .map(|d| i64::from(*d) * 86_400_000_000)
+ .collect();
+ let expected = RecordBatch::try_new(
+ Arc::new(schema_to_arrow_schema(&new_schema).unwrap()),
+ vec![
+ Arc::new(TimestampMicrosecondArray::from(micros.clone())),
+ Arc::new(TimestampNanosecondArray::from(
+ micros.iter().map(|us| us * 1000).collect::<Vec<_>>(),
Review Comment:
The test uses magic numbers for time unit conversion (`86_400_000_000`
microseconds/day and `* 1000` for nanos). Introduce named constants (e.g.,
`MICROS_PER_DAY`, `NANOS_PER_MICRO`) or a small helper to make the intent/unit
semantics explicit and reduce the chance of accidental unit mistakes in future
edits.
##########
crates/iceberg/src/arrow/reader/projection.rs:
##########
@@ -3075,4 +3079,88 @@ message schema {
assert_eq!(fallback.len(), 1);
assert_top_list(&fallback[0]);
}
+
+ /// Reads a file that stores `date` columns through a schema that promotes
them to
+ /// `timestamp` and `timestamp_ns`, with an unpromoted column projected
alongside them.
+ #[tokio::test]
+ async fn test_read_date_promoted_to_timestamp_with_other_columns() {
+ use arrow_array::{
+ Date32Array, Int32Array, TimestampMicrosecondArray,
TimestampNanosecondArray,
+ };
+
+ use crate::arrow::schema_to_arrow_schema;
+
+ fn schema(d: PrimitiveType, e: PrimitiveType) -> Schema {
+ Schema::builder()
+ .with_fields(vec![
+ NestedField::optional(1, "d", Type::Primitive(d)).into(),
+ NestedField::required(2, "e", Type::Primitive(e)).into(),
+ NestedField::optional(3, "a",
Type::Primitive(PrimitiveType::Int)).into(),
+ ])
+ .build()
+ .unwrap()
+ }
+
+ let days = [18000, -1, 0];
+ let ints = Arc::new(Int32Array::from(vec![1, 2, 3])) as ArrayRef;
+ let old_schema = schema(PrimitiveType::Date, PrimitiveType::Date);
+ let old_batch = RecordBatch::try_new(
+ Arc::new(schema_to_arrow_schema(&old_schema).unwrap()),
+ vec![
+ Arc::new(Date32Array::from(days.to_vec())),
+ Arc::new(Date32Array::from(days.to_vec())),
+ ints.clone(),
+ ],
+ )
+ .unwrap();
+ let new_schema = Arc::new(schema(PrimitiveType::Timestamp,
PrimitiveType::TimestampNs));
+ let micros: Vec<i64> = days
+ .iter()
+ .map(|d| i64::from(*d) * 86_400_000_000)
+ .collect();
+ let expected = RecordBatch::try_new(
+ Arc::new(schema_to_arrow_schema(&new_schema).unwrap()),
+ vec![
+ Arc::new(TimestampMicrosecondArray::from(micros.clone())),
+ Arc::new(TimestampNanosecondArray::from(
+ micros.iter().map(|us| us * 1000).collect::<Vec<_>>(),
+ )),
+ ints,
+ ],
+ )
+ .unwrap();
+
+ let tmp_dir = TempDir::new().unwrap();
+ let file_path = format!("{}/date.parquet",
tmp_dir.path().to_str().unwrap());
+ let mut writer =
+ ArrowWriter::try_new(File::create(&file_path).unwrap(),
old_batch.schema(), None)
Review Comment:
Building `file_path` via `format!(\"{}/...\",
tmp_dir.path().to_str().unwrap())` is brittle (depends on UTF-8 paths and
manual separators). Prefer `tmp_dir.path().join(\"date.parquet\")` and pass a
`Path`/`PathBuf` (or convert via `.to_string_lossy()` only if the downstream
API requires a `String`) to make the test more portable.
--
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]
