You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "alamb (via GitHub)" <gi...@apache.org> on 2023/04/03 11:34:11 UTC
[GitHub] [arrow-datafusion] alamb opened a new pull request, #5843: Change `FileFormat` to depends on `TaskContext` rather than `SessionState`
alamb opened a new pull request, #5843:
URL: https://github.com/apache/arrow-datafusion/pull/5843
# Which issue does this PR close?
Part of https://github.com/apache/arrow-datafusion/issues/1754
# Rationale for this change
I am trying to extract the physical_plan code into its own crate; and to do so I need to remove the circular dependencies between core --> datasource --> execution --> datasource
Since `SessionState` is staying in `datafusion` core crate, in order to pull out datasource into its own crate (and only depend on datafusion_execution) it can not depend on `SessionState`
Conveniently, `TaskContext` is designed to be exactly the part of `SessionState` needed for execution
See more details in https://github.com/apache/arrow-datafusion/issues/1754#issuecomment-1452438453
# What changes are included in this PR?
1. Change `FileFormat` to depends on `TaskContext` rather than SessionState
# Are these changes tested?
Covered by existing tests
# Are there any user-facing changes?
Technically the `FileFormat` / `ListingTable` is a public API; However, I don't think the `FileFormat` is widely used outside of DataFusion so I expect this to have minimal impact on users
I expect the change of TableProvider to use `TaskContext` to be the most disruptive and I am saving it for last.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5843: Change `FileFormat` to depends on `TaskContext` rather than `SessionState`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5843:
URL: https://github.com/apache/arrow-datafusion/pull/5843#issuecomment-1588261043
I am taking a different direction for now, so closing this PR -- see https://github.com/apache/arrow-datafusion/issues/1754 for latest plan
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on a diff in pull request #5843: Change `FileFormat` to depends on `TaskContext` rather than `SessionState`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #5843:
URL: https://github.com/apache/arrow-datafusion/pull/5843#discussion_r1155848115
##########
datafusion/core/src/datasource/file_format/avro.rs:
##########
@@ -48,7 +48,7 @@ impl FileFormat for AvroFormat {
async fn infer_schema(
&self,
- _state: &SessionState,
+ _task_ctx: &TaskContext,
Review Comment:
The point of this PR is to change the `SessionState` references in these traits to `TaskContext`
##########
datafusion/core/src/execution/context.rs:
##########
@@ -1176,7 +1178,14 @@ impl QueryPlanner for DefaultQueryPlanner {
}
}
-/// Execution context for registering data sources and executing queries
+/// Holds state needed for planning queries within the context of a Session.
Review Comment:
Tried to clarify SessionState vs TaskContext
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb closed pull request #5843: Change `FileFormat` to depends on `TaskContext` rather than `SessionState`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #5843: Change `FileFormat` to depends on `TaskContext` rather than `SessionState`
URL: https://github.com/apache/arrow-datafusion/pull/5843
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] alamb commented on pull request #5843: Change `FileFormat` to depends on `TaskContext` rather than `SessionState`
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #5843:
URL: https://github.com/apache/arrow-datafusion/pull/5843#issuecomment-1496584283
> Perhaps I am missing something here, but given FileFormat and the closely related ListingTable are part of query planning, and not execution, I'm not sure why they would depend on TaskContext which is the execution state?
The update here is that @tustvold and I talked through this, and I think we were using different terms / ideas of what "planning" and "execution" mean.
I will attempt to add some more docs to make this clearer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-datafusion] tustvold commented on pull request #5843: Change `FileFormat` to depends on `TaskContext` rather than `SessionState`
Posted by "tustvold (via GitHub)" <gi...@apache.org>.
tustvold commented on PR #5843:
URL: https://github.com/apache/arrow-datafusion/pull/5843#issuecomment-1494328695
Perhaps I am missing something here, but given FileFormat and the closely related ListingTable are part of query planning, and not execution, I'm not sure why they would depend on TaskContext which is the execution state?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org