-
Notifications
You must be signed in to change notification settings - Fork 307
feat: add experimental remote HDFS support for native DataFusion reader #1359
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
Changes from all commits
f099ff8
36600d2
19c8bed
d9b33aa
ad030ca
cb53370
9b9d1e1
66b6e7f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -74,7 +74,7 @@ use datafusion_physical_expr::aggregate::{AggregateExprBuilder, AggregateFunctio | |
|
|
||
| use crate::execution::shuffle::CompressionCodec; | ||
| use crate::execution::spark_plan::SparkPlan; | ||
| use crate::parquet::parquet_support::SparkParquetOptions; | ||
| use crate::parquet::parquet_support::{register_object_store, SparkParquetOptions}; | ||
| use crate::parquet::schema_adapter::SparkSchemaAdapterFactory; | ||
| use datafusion::datasource::listing::PartitionedFile; | ||
| use datafusion::datasource::physical_plan::parquet::ParquetExecBuilder; | ||
|
|
@@ -106,7 +106,6 @@ use datafusion_common::{ | |
| tree_node::{Transformed, TransformedResult, TreeNode, TreeNodeRecursion, TreeNodeRewriter}, | ||
| JoinType as DFJoinType, ScalarValue, | ||
| }; | ||
| use datafusion_execution::object_store::ObjectStoreUrl; | ||
| use datafusion_expr::type_coercion::other::get_coerce_type_for_case_expression; | ||
| use datafusion_expr::{ | ||
| AggregateUDF, ReturnTypeArgs, ScalarUDF, WindowFrame, WindowFrameBound, WindowFrameUnits, | ||
|
|
@@ -1165,12 +1164,9 @@ impl PhysicalPlanner { | |
| )) | ||
| }); | ||
|
|
||
| let object_store = object_store::local::LocalFileSystem::new(); | ||
| // register the object store with the runtime environment | ||
| let url = Url::try_from("file://").unwrap(); | ||
| self.session_ctx | ||
| .runtime_env() | ||
| .register_object_store(&url, Arc::new(object_store)); | ||
| // By default, local FS object store registered | ||
| // if `hdfs` feature enabled then HDFS file object store registered | ||
| let object_store_url = register_object_store(Arc::clone(&self.session_ctx))?; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we update this function (get_file_path) as well?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thats a good point, to verify it we probably need to read Iceberg from HDFS which can be done in #1367
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to wait for actual iceberg integration. CometScan will use COMPAT_ICEBERG if the configuration is set (That's how we are able to run the unit tests).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @comphead we can log a follow up issue to update
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @parthchandra lets create a followup ticket. Appreciate if you do it as I'm afraid I can miss some Iceberg details in ticket description
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. #1407. There's no detail in the PR. Can you assign to me if possible, and I'll remember to take care of it. |
||
|
|
||
| // Generate file groups | ||
| let mut file_groups: Vec<Vec<PartitionedFile>> = | ||
|
|
@@ -1229,8 +1225,6 @@ impl PhysicalPlanner { | |
|
|
||
| // TODO: I think we can remove partition_count in the future, but leave for testing. | ||
| assert_eq!(file_groups.len(), partition_count); | ||
|
|
||
| let object_store_url = ObjectStoreUrl::local_filesystem(); | ||
| let partition_fields: Vec<Field> = partition_schema | ||
| .fields() | ||
| .iter() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,16 +15,19 @@ | |
| // specific language governing permissions and limitations | ||
| // under the License. | ||
|
|
||
| use crate::execution::operators::ExecutionError; | ||
| use arrow::{ | ||
| array::{cast::AsArray, types::Int32Type, Array, ArrayRef}, | ||
| compute::{cast_with_options, take, CastOptions}, | ||
| util::display::FormatOptions, | ||
| }; | ||
| use arrow_array::{DictionaryArray, StructArray}; | ||
| use arrow_schema::DataType; | ||
| use datafusion::prelude::SessionContext; | ||
| use datafusion_comet_spark_expr::utils::array_with_timezone; | ||
| use datafusion_comet_spark_expr::EvalMode; | ||
| use datafusion_common::{Result as DataFusionResult, ScalarValue}; | ||
| use datafusion_execution::object_store::ObjectStoreUrl; | ||
| use datafusion_expr::ColumnarValue; | ||
| use std::collections::HashMap; | ||
| use std::{fmt::Debug, hash::Hash, sync::Arc}; | ||
|
|
@@ -195,3 +198,39 @@ fn cast_struct_to_struct( | |
| _ => unreachable!(), | ||
| } | ||
| } | ||
|
|
||
| // Default object store which is local filesystem | ||
| #[cfg(not(feature = "hdfs"))] | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| pub(crate) fn register_object_store( | ||
| session_context: Arc<SessionContext>, | ||
| ) -> Result<ObjectStoreUrl, ExecutionError> { | ||
| let object_store = object_store::local::LocalFileSystem::new(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't have to be only a local file system.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It depends on the feature enabled for the Comet. LocalFileSystem is by default if no specific features selected. This allows to plugin other features like S3, etc This particular method is responsible for no remote feature selected e.g. for local filesystem. |
||
| let url = ObjectStoreUrl::parse("file://")?; | ||
| session_context | ||
| .runtime_env() | ||
| .register_object_store(url.as_ref(), Arc::new(object_store)); | ||
| Ok(url) | ||
| } | ||
|
|
||
| // HDFS object store | ||
| #[cfg(feature = "hdfs")] | ||
| pub(crate) fn register_object_store( | ||
| session_context: Arc<SessionContext>, | ||
| ) -> Result<ObjectStoreUrl, ExecutionError> { | ||
| // TODO: read the namenode configuration from file schema or from spark.defaultFS | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to register object store from
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @wForget I'm not sure I'm getting it, do you mean the better place to register the object store will be inside
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, is it possible that native scan paths correspond to multiple object stores or are different from spark.defaultFs?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for HDFS/S3 the default fs can be taken from
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Sometimes I also access other hdfs ns like:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is interesting scenario, I'll add a separate test case for this |
||
| let url = ObjectStoreUrl::parse("hdfs://namenode:9000")?; | ||
| if let Some(object_store) = | ||
| datafusion_comet_objectstore_hdfs::object_store::hdfs::HadoopFileSystem::new(url.as_ref()) | ||
| { | ||
| session_context | ||
| .runtime_env() | ||
| .register_object_store(url.as_ref(), Arc::new(object_store)); | ||
|
|
||
| return Ok(url); | ||
| } | ||
|
|
||
| Err(ExecutionError::GeneralError(format!( | ||
| "HDFS object store cannot be created for {}", | ||
| url | ||
| ))) | ||
| } | ||
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.
nit: is JAVA_HOME still the requirement?