Skip to content

Conversation

@xxhZs
Copy link
Collaborator

@xxhZs xxhZs commented May 26, 2025

support datafusion iceberg transform physical expr
and mv all datafusion impl to new dir

This is for implementing repartitioning, which is part of #20.

@xxhZs xxhZs requested review from Li0k, chenzl25 and Copilot and removed request for chenzl25 and Copilot July 3, 2025 08:13
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR refactors existing DataFusion executors into a new datafusion_impl directory and adds support for Iceberg column transform expressions in physical plans.

  • Moved file_scan_task_table_provider and iceberg_file_task_scan modules under datafusion_impl
  • Updated import paths in datafusion_processor and iceberg_file_task_scan
  • Introduced IcebergTransformPhysicalExpr to evaluate Iceberg Transform functions in DataFusion

Reviewed Changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
core/src/executor/datafusion/mod.rs Replaced direct module declarations with pub mod datafusion_impl
core/src/executor/datafusion/datafusion_processor.rs Updated import path for IcebergFileScanTaskTableProvider
core/src/executor/datafusion/datafusion_impl/mod.rs Added new datafusion_impl module
core/src/executor/datafusion/datafusion_impl/iceberg_physical_expr.rs Added IcebergTransformPhysicalExpr implementation and tests
core/src/executor/datafusion/datafusion_impl/iceberg_file_task_scan.rs Updated import path for SYS_HIDDEN_SEQ_NUM
Comments suppressed due to low confidence (1)

core/src/executor/datafusion/datafusion_impl/iceberg_physical_expr.rs:53

  • The debug struct name doesn’t match the actual struct (IcebergTransformPhysicalExpr). Update to f.debug_struct("IcebergTransformPhysicalExpr") for consistency.
        f.debug_struct("IcebergPhysicalExpr")

let res_array = self
.transform
.transform(batch.column(self.index).clone())
.unwrap();
Copy link

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling .unwrap() on the transform result may cause a panic at runtime. Consider propagating errors (e.g., using ? and mapping to a DataFusion error) instead of unwrapping.

Suggested change
.unwrap();
.map_err(to_datafusion_error)?;

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants