-
Notifications
You must be signed in to change notification settings - Fork 78
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
Upgrade to DataFusion 16.0.0 #115
Conversation
let df = if let Some(schema) = schema { | ||
options.schema = Some(&schema.0); | ||
let result = self.ctx.read_json(path, options); | ||
wait_for_future(py, result).map_err(DataFusionError::from)? | ||
} else { | ||
let result = self.ctx.read_json(path, options); | ||
wait_for_future(py, result).map_err(DataFusionError::from)? | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was necessary because options.schema
holds a reference to a schema now so we need to wait for the future to complete before ownership can be released.
pub fn new(df: Arc<DataFrame>) -> Self { | ||
Self { df } | ||
pub fn new(df: DataFrame) -> Self { | ||
Self { df: Arc::new(df) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pyo3 methods require &self
and cannot take self
, so my solution was to introduce an Arc
here.
@andygrove given that the remaining test failures preventing this PR from getting merged all all related to |
Actually I see that |
@jdye64 I have now fixed all the regressions. |
@francis-du fyi |
LGTM. That was a bear. Thanks for tackling it. |
Which issue does this PR close?
Closes #134
Rationale for this change
Upgrade to pick up latest features
What changes are included in this PR?
Are there any user-facing changes?