gabotechs commented on code in PR #14:
URL: https://github.com/apache/datafusion-iceberg/pull/14#discussion_r4079410831
##########
crates/datafusion/src/physical_plan/project.rs:
##########
@@ -91,38 +87,84 @@ pub fn project_with_partition(
projection_exprs.push((column_expr, field.name().clone()));
}
- let partition_expr = Arc::new(PartitionExpr::new(calculator,
partition_spec.clone()));
+ let partition_expr = Arc::new(PartitionExpr::try_new(
+ partition_spec.clone(),
+ table_schema.clone(),
+ )?);
projection_exprs.push((partition_expr,
PROJECTED_PARTITION_VALUE_COLUMN.to_string()));
let projection = ProjectionExec::try_new(projection_exprs, input)?;
Ok(Arc::new(projection))
}
/// PhysicalExpr implementation for partition value calculation
+///
+/// The [`PartitionValueCalculator`] cannot be serialized, so the spec and
schema
+/// it was built from are retained: [`Self::try_new`] rebuilds from those.
#[derive(Debug, Clone)]
-struct PartitionExpr {
+pub struct PartitionExpr {
calculator: Arc<PartitionValueCalculator>,
partition_spec: Arc<PartitionSpec>,
+ table_schema: SchemaRef,
Review Comment:
It'd be nice to import this as `IcebergSchemaRef` in order to not mix it
with with `arrow::SchemaRef`. Unfortunately iceberg_rust decided to choose
colliding names with a lot of `arrow` primitives, so whatever allows us to
differentiate between them is welcome. Fully qualifying the struct here should
also work `iceberg::spec::SchemaRef`
##########
crates/datafusion/src/physical_plan/project.rs:
##########
@@ -181,9 +223,11 @@ impl std::fmt::Display for PartitionExpr {
impl std::hash::Hash for PartitionExpr {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
- // Two PartitionExpr are equal if they share the same calculator and
partition_spec Arcs
- Arc::as_ptr(&self.calculator).hash(state);
- Arc::as_ptr(&self.partition_spec).hash(state);
Review Comment:
🤔 I wonder why it was like this before. It sounds like playing a dangerous
game to rely on pointer equivalence for this, so maybe they had a reason for
it? I see the same in `PartialEq::eq`, it'd be nice to find the reason for this
before committing to change, we might be missing something.
##########
crates/datafusion/src/physical_plan/project.rs:
##########
@@ -199,6 +243,14 @@ mod tests {
use super::*;
+ fn hash_of(expr: &PartitionExpr) -> u64 {
+ use std::collections::hash_map::DefaultHasher;
+ use std::hash::{Hash, Hasher};
Review Comment:
Could we place imports at the top of the `tests` module like the rest?
--
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]
