Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prototype: Directly call parent node's function #161

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

nikhil96sher
Copy link
Collaborator

@nikhil96sher nikhil96sher commented Dec 7, 2022

Initial prototype of directly calling parent node's function instead of passing data through channels.

Things currently working:

  1. Can directly call parent node's function instead of passing data through channel.
  2. Can perform this with multiple downstream nodes in parallel.

Things to add:

  1. Eager/Lazy option when creating ExecutionNode. Eager execution node starts the execution in another thread. Lazy execution node doesn't start any execution.
  2. Interior Mutability while keeping ExecutionNode immutably borrowed. Important because the same node needs to be borrowed in child nodes. So, we cannot have it borrowed mutably.

Copy link
Collaborator

@mIXs222 mIXs222 left a comment

Choose a reason for hiding this comment

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

Few things i saw during meeting

// Operation 1: Reader [] => This is a generator for input data.
#[derive(Getters, Setters)]
pub struct Reader {
node: Option<Weak<RwLock<ExecutionNode>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

node: Weak<RwLock<ExecutionNode>>, ?

Then we can check during upgrade like match node.upgrade() {Some(...) => ..., None => ...}

// Operation 2: Appender [] => Can have custom map implementations.
#[derive(Getters, Setters)]
pub struct Mapper {
node: Option<Weak<RwLock<ExecutionNode>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

node: Weak<RwLock<ExecutionNode>>, ?

Copy link
Collaborator Author

@nikhil96sher nikhil96sher Dec 8, 2022

Choose a reason for hiding this comment

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

I think we can get rid of the Option.
I initially added it to first create operation without a node. Then create the execution node with this operation while updating the node in the operation.
But Weak itself should be good enough to support that.

Comment on lines +37 to +42
let value = self.input_data.get(partition_num);
if let Some(x) = value {
Some(*x)
} else {
None
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just return self.input_data.get(partition_num) ?

Copy link
Collaborator Author

@nikhil96sher nikhil96sher Dec 8, 2022

Choose a reason for hiding this comment

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

So, turns out .get() on a Vec<Item> returns Option<&Item>.
Directly returning the value mismatches the signature (Option<i64> vs Option<&i64>). That's why did the reconstruction.
But there should be some better way.

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