gabotechs commented on code in PR #65:
URL: https://github.com/apache/datafusion-iceberg/pull/65#discussion_r4311169991
##########
crates/datafusion/src/test_utils/harness.rs:
##########
@@ -15,8 +15,12 @@
// specific language governing permissions and limitations
// under the License.
+#[cfg(test)]
+use std::ops::Range;
Review Comment:
I'd avoid flooding this file with #[cfg(test)] macros, this whole file is
already for tests itself.
##########
Cargo.toml:
##########
@@ -37,8 +37,8 @@ datafusion = "55.0.0"
datafusion-cli = "55.0.0"
iceberg-datafusion = { path = "crates/datafusion" }
datafusion-sqllogictest = "55.0.0"
-iceberg = { git = "https://github.com/apache/iceberg-rust";, rev =
"8cb2adeddb4f8da1ca7bd86ca303337f011c12e2" }
-iceberg-catalog-rest = { git = "https://github.com/apache/iceberg-rust";, rev =
"8cb2adeddb4f8da1ca7bd86ca303337f011c12e2" }
+iceberg = { git = "https://github.com/apache/iceberg-rust";, rev =
"3c952e1719a4902334e44b648698999cf3a4214d" }
+iceberg-catalog-rest = { git = "https://github.com/apache/iceberg-rust";, rev =
"3c952e1719a4902334e44b648698999cf3a4214d" }
Review Comment:
I'd avoid mixing a PR that upgrades `iceberg-rust` with anything else. Can
we bump this separately?
##########
crates/datafusion/src/test_utils/harness.rs:
##########
@@ -42,18 +46,23 @@ const WAREHOUSE_URI: &str = "s3://iceberg-test/warehouse/";
pub struct IcebergTestHarness {
ctx: SessionContext,
+ #[cfg(test)]
+ parquet_read_bytes: Arc<AtomicU64>,
Review Comment:
DataFusion is (or should be) already collecting this as metrics in the
`IcebergDataSource` struct itself as metrics. DataFusion's metrics is the
standard way in which we can inspect how many bytes where scanned from a
`DataSource` implementation, so rather than inventing another channel for
surfacing this, I'd just use the metrics provided in the node.
I'm unaware if we are already collecting the metrics in the node or not. If
we are not, we should do that in another PR before this one.
##########
crates/datafusion/src/write/project.rs:
##########
@@ -732,16 +732,16 @@ mod tests {
.identifier(TableIdent::from_strs(["test", "table"]).unwrap())
.file_io(FileIO::new_with_fs())
.metadata_location("/test/metadata.json")
- .runtime(test_runtime())
+ .runtime(Runtime::current())
.build()
.unwrap();
let result = project_with_partition(input, &table);
assert!(result.is_ok(), "Schema validation should pass");
}
- #[test]
- fn test_schema_validation_mismatched_schemas() {
+ #[tokio::test]
+ async fn test_schema_validation_mismatched_schemas() {
Review Comment:
Why would this change be needed? tests here do not contain async code. Is
there any chance we can rollback all these changes?
##########
crates/datafusion/src/test_utils/harness.rs:
##########
@@ -177,3 +208,135 @@ impl Storage for FixtureStorage {
Ok(OutputFile::new(Arc::new(self.clone()), path.to_string()))
}
}
+
+#[cfg(test)]
+struct CountingFileRead {
+ reader: Box<dyn FileRead>,
+ bytes: Arc<AtomicU64>,
+}
+
+#[cfg(test)]
+#[async_trait]
+impl FileRead for CountingFileRead {
+ async fn read(&self, range: Range<u64>) -> IcebergResult<Bytes> {
+ let bytes = self.reader.read(range).await?;
+ self.bytes.fetch_add(bytes.len() as u64, Ordering::Relaxed);
+ Ok(bytes)
+ }
+}
+
+#[cfg(test)]
+impl IcebergTestHarness {
+ pub(crate) fn context(&self) -> &SessionContext {
+ &self.ctx
+ }
+
+ pub(crate) fn take_parquet_read_bytes(&self) -> u64 {
+ self.parquet_read_bytes.swap(0, Ordering::Relaxed)
+ }
+
+ pub(crate) async fn with_parquet(
+ schema: iceberg::spec::Schema,
+ batch: &datafusion::arrow::array::RecordBatch,
+ properties: parquet::file::properties::WriterProperties,
+ catalog_backed: bool,
+ ) -> Result<(Self, tempfile::TempDir)> {
Review Comment:
This method shows some traits of un-curated LLM output. If what we need is
so custom that this new `with_parquet` method is required for building an
`IcebergTestHarness`, I'd just skip using `IcebergTestHarness` at all.
If rather than building custom parquet files we could just reuse the
existing ones in the project (the taxi dataset) for testing what this PR ships,
that'd be ideal.
--
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]
