Skip to content

Refactor Scan I/O plan#731

Merged
eddyxu merged 20 commits intomainfrom
lei/refactor_plan
Mar 24, 2023
Merged

Refactor Scan I/O plan#731
eddyxu merged 20 commits intomainfrom
lei/refactor_plan

Conversation

@eddyxu
Copy link
Copy Markdown
Member

@eddyxu eddyxu commented Mar 24, 2023

  • Simplify and formalize the excution plan creation
  • Add tests for each i/o execution case

Closes #687

@eddyxu eddyxu force-pushed the lei/refactor_plan branch from d54a930 to 11bbb91 Compare March 24, 2023 06:43
@eddyxu eddyxu force-pushed the lei/refactor_plan branch from 1880c6a to 2fb8a87 Compare March 24, 2023 18:43
@eddyxu eddyxu requested a review from changhiskhan March 24, 2023 18:47
@eddyxu eddyxu marked this pull request as ready for review March 24, 2023 18:47
@eddyxu eddyxu requested a review from gsilvestrin March 24, 2023 18:54
@eddyxu eddyxu self-assigned this Mar 24, 2023
@eddyxu eddyxu added the rust Rust related tasks label Mar 24, 2023
Copy link
Copy Markdown
Contributor

@changhiskhan changhiskhan left a comment

Choose a reason for hiding this comment

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

2 minor questions

Comment thread rust/src/dataset/scanner.rs Outdated
}

fn get_exec_columns(plan: &dyn ExecutionPlan) -> Vec<String> {
plan.schema()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

should we make a field_names method in schema?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is Arrow schema, guess we can add this method via ArrowSchemaExt.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Done

vec![]
};
let knn_idx = indices.iter().find(|i| i.fields.contains(&column_id));
if let Some(index) = knn_idx {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

is this just copy paste? any changes?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Moved the filters out, so the filters consolidates with the rest of exec path.

@eddyxu eddyxu merged commit f99a5ee into main Mar 24, 2023
@eddyxu eddyxu deleted the lei/refactor_plan branch March 24, 2023 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

rust Rust related tasks

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Formalize io exec node contracts

2 participants