Skip to content

Conversation

@Weijun-H
Copy link
Member

Which issue does this PR close?

Closes #1517

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

@github-actions github-actions bot added the core Core DataFusion crate label Apr 18, 2023
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @Weijun-H

I realize #1517 says "ExecutionContext" but this code is in the experimental "scheduler"

I wonder if #1517 meant to say SessionContext or SessionState or TaskContext (or perhaps the code has been changed since the ticket was filed)


/// The shared state of all [`Task`] created from the same [`PipelinePlan`]
#[derive(Debug)]
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a function like

pub fn version(&self) -> &str {
...
}

Which would avoid the need for allow(dead_code) perhaps

@Weijun-H
Copy link
Member Author

Thanks @Weijun-H

I realize #1517 says "ExecutionContext" but this code is in the experimental "scheduler"

I wonder if #1517 meant to say SessionContext or SessionState or TaskContext (or perhaps the code has been changed since the ticket was filed)

I vote for SessionState, because it is the execution context for executing queries. What do you think? 🤔

@Weijun-H Weijun-H force-pushed the add-version-field-to-ExecutionContext branch from f8d81a0 to 8b2154d Compare April 20, 2023 21:12
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @Weijun-H !


/// Return version
pub fn version(&self) -> &str {
&self.version
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you could just call env!("CARGO_PKG_VERSION") here -- is there any reason to create a string to do so?

Suggested change
&self.version
env!("CARGO_PKG_VERSION")

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thank you @Weijun-H 👍

@alamb alamb merged commit 9775305 into apache:main Apr 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core Core DataFusion crate

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add version field to ExecutionContext

2 participants