You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "comphead (via GitHub)" <gi...@apache.org> on 2025/01/31 01:00:11 UTC
[PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
comphead opened a new pull request, #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #1337.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## How are these changes tested?
Manually starting a remote hdfs cluster and running
```
test("get_struct_field with DataFusion ParquetExec - simple case - remote HDFS") {
Seq("parquet").foreach { v1List =>
withSQLConf(
SQLConf.USE_V1_SOURCE_LIST.key -> v1List,
CometConf.COMET_ENABLED.key -> "true",
CometConf.COMET_EXPLAIN_FALLBACK_ENABLED.key -> "false",
CometConf.COMET_NATIVE_SCAN_IMPL.key -> CometConf.SCAN_NATIVE_DATAFUSION,
"spark.hadoop.fs.defaultFS" -> "hdfs://namenode:9000",
"spark.hadoop.dfs.client.use.datanode.hostname" -> "true",
"dfs.client.use.datanode.hostname" -> "true") {
val df = spark.read
.parquet("hdfs://namenode:9000/user/test4")
.select("id", "first_name", "personal_info")
df.printSchema()
df.explain("formatted")
df.show(false)
// checkSparkAnswerAndOperator(df.select("nested1.id"))
}
}
}
```
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#issuecomment-2664351633
Thanks everyone
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936519276
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1864,40 @@ fn trim_end(s: &str) -> &str {
}
}
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<(), ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
+ let url = Url::try_from("file://").unwrap();
+ session_context
+ .runtime_env()
+ .register_object_store(&url, Arc::new(object_store));
+ Ok(())
+}
+
+#[cfg(feature = "hdfs")]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<(), ExecutionError> {
+ // TODO: read the namenode configuration from file schema or from spark.defaultFS
+ let url = Url::try_from("hdfs://namenode:9000").unwrap();
Review Comment:
this will be addressed in https://github.com/apache/datafusion-comet/issues/1360
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936518217
##########
native/core/Cargo.toml:
##########
@@ -77,6 +77,7 @@ datafusion-comet-proto = { workspace = true }
object_store = { workspace = true }
url = { workspace = true }
chrono = { workspace = true }
+datafusion-objectstore-hdfs = { git = "https://github.com/comphead/datafusion-objectstore-hdfs", branch = "master", optional = true }
Review Comment:
@andygrove I'm keeping the updated HDFS object storage in personal repo for now, let me know if there any concerns
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1943985797
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1863,42 @@ fn trim_end(s: &str) -> &str {
}
}
+// Default object store which is local filesystem
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<ObjectStoreUrl, ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
+ 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
Review Comment:
Thanks @wForget I'm not sure I'm getting it, do you mean the better place to register the object store will be inside `file_partitions` iterator loop ?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#issuecomment-2631714236
Thanks @parthchandra the integration test will be addressed as part of #1367. We also need to think should it be a separate flow in CI?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1956937048
##########
native/core/src/execution/planner.rs:
##########
@@ -1155,12 +1154,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))?;
Review Comment:
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
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#issuecomment-2660574425
oops, @comphead would you mind merging the latest main into this PR branch in order to resolve the conflict?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936518708
##########
native/core/src/execution/planner.rs:
##########
@@ -1220,7 +1217,7 @@ 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 object_store_url = ObjectStoreUrl::parse("hdfs://namenode:9000").unwrap();
Review Comment:
this will be addressed in #1360
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1945908388
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1863,42 @@ fn trim_end(s: &str) -> &str {
}
}
+// Default object store which is local filesystem
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<ObjectStoreUrl, ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
+ 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
Review Comment:
> for HDFS/S3 the default fs can be taken from spark.hadoop.fs.defaultFS parameter.
Sometimes I also access other hdfs ns like:
```
select * from `parquet`.`hdfs://other-ns:8020/warehouse/db/table`
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1956879942
##########
native/core/src/execution/planner.rs:
##########
@@ -1155,12 +1154,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))?;
Review Comment:
@comphead we can log a follow up issue to update `get_file_path` if you like.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936553571
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1864,40 @@ fn trim_end(s: &str) -> &str {
}
}
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<(), ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
Review Comment:
It depends on the feature enabled for the Comet. LocalFileSystem is by default if no specific features selected.
the annotation on this method is
```
#[cfg(not(feature = "hdfs"))]
```
This allows to plugin other features like S3, etc
This particular method is responsible for no remote feature selected e.g. for local filesystem.
If a feature selected the conditional compilation will register an object store related to the feature, like HDFS or S3
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1943975592
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1863,42 @@ fn trim_end(s: &str) -> &str {
}
}
+// Default object store which is local filesystem
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<ObjectStoreUrl, ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
+ 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
Review Comment:
Do we need to register object store from `native_scan.file_partitions`?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1941967578
##########
Makefile:
##########
@@ -95,7 +98,7 @@ release-linux: clean
cd native && RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit" cargo build --release
./mvnw install -Prelease -DskipTests $(PROFILES)
release:
- cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+ cd native && RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" && RUSTFLAGS=$$RUSTFLAGS cargo build --release $(FEATURES_ARG)
Review Comment:
```
RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" && RUSTFLAGS=$$RUSTFLAGS cargo...
```
did you mean
```
RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" cargo...
```
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936520898
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1864,40 @@ fn trim_end(s: &str) -> &str {
}
}
+#[cfg(not(feature = "hdfs"))]
Review Comment:
`hdfs` cargo feature makes a conditional compilation if hdfs needed
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936551696
##########
native/core/src/execution/planner.rs:
##########
@@ -1220,7 +1217,7 @@ 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 object_store_url = ObjectStoreUrl::parse("hdfs://namenode:9000").unwrap();
Review Comment:
Thanks @parthchandra it is already fixed.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1939820038
##########
Makefile:
##########
@@ -95,7 +98,7 @@ release-linux: clean
cd native && RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit" cargo build --release
./mvnw install -Prelease -DskipTests $(PROFILES)
release:
- cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+ cd native && RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" && RUSTFLAGS=$$RUSTFLAGS cargo build --release $(FEATURES_ARG)
Review Comment:
Makefile syntax lightly different from bash from what I learned.
so in order to access environment variable created on fly it is needed to access it with $$.
It gets created on fly to concatenate release specific RUSTFLAGS with ones that user can set
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead merged PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1939889625
##########
native/core/src/execution/planner.rs:
##########
@@ -1155,12 +1154,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))?;
Review Comment:
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).
##########
Makefile:
##########
@@ -95,7 +98,7 @@ release-linux: clean
cd native && RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit" cargo build --release
./mvnw install -Prelease -DskipTests $(PROFILES)
release:
- cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+ cd native && RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" && RUSTFLAGS=$$RUSTFLAGS cargo build --release $(FEATURES_ARG)
Review Comment:
Thanks. Learnt something new today :)
##########
native/core/Cargo.toml:
##########
@@ -77,6 +77,7 @@ datafusion-comet-proto = { workspace = true }
object_store = { workspace = true }
url = { workspace = true }
chrono = { workspace = true }
+datafusion-objectstore-hdfs = { git = "https://github.com/comphead/datafusion-objectstore-hdfs", branch = "master", optional = true }
Review Comment:
Is there an expected timeline for when we can move to an official release? Meantime, since we have pointed to a personal repo in the past, it is reasonable to do so for this as well (especially since this is already behind some configuration flags).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1939820038
##########
Makefile:
##########
@@ -95,7 +98,7 @@ release-linux: clean
cd native && RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit" cargo build --release
./mvnw install -Prelease -DskipTests $(PROFILES)
release:
- cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+ cd native && RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" && RUSTFLAGS=$$RUSTFLAGS cargo build --release $(FEATURES_ARG)
Review Comment:
Makefile syntax lightly different from bash from what I learned.
so in order to access environment variable created on fly it is needed to access it with $$
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1939760671
##########
native/core/src/execution/planner.rs:
##########
@@ -1155,12 +1154,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))?;
Review Comment:
Should we update this function ([get_file_path](https://github.com/apache/datafusion-comet/blob/996362e78d497c02542f1e29dbb7cba3ec16f64c/native/core/src/parquet/util/jni.rs#L214C8-L214C21)) as well?
It's currently used by `NATIVE_ICEBERG_COMPAT` but the goal is to unify it with `COMET_DATAFUSION`.
##########
Makefile:
##########
@@ -95,7 +98,7 @@ release-linux: clean
cd native && RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit" cargo build --release
./mvnw install -Prelease -DskipTests $(PROFILES)
release:
- cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+ cd native && RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" && RUSTFLAGS=$$RUSTFLAGS cargo build --release $(FEATURES_ARG)
Review Comment:
`$$RUSTFLAGS` ? Never seen this pattern before. What does this do?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1942084981
##########
native/core/Cargo.toml:
##########
@@ -77,6 +77,7 @@ datafusion-comet-proto = { workspace = true }
object_store = { workspace = true }
url = { workspace = true }
chrono = { workspace = true }
+datafusion-objectstore-hdfs = { git = "https://github.com/comphead/datafusion-objectstore-hdfs", branch = "master", optional = true }
Review Comment:
Will be addressed in #1368
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1958826427
##########
docs/source/user-guide/datasources.md:
##########
@@ -35,3 +35,81 @@ converted into Arrow format, allowing native execution to happen after that.
Comet does not provide native JSON scan, but when `spark.comet.convert.json.enabled` is enabled, data is immediately
converted into Arrow format, allowing native execution to happen after that.
+
+# Supported Storages
+
+## Local
+In progress
+
+## HDFS
+
+Apache DataFusion Comet native reader seamlessly scans files from remote HDFS for [supported formats](#supported-spark-data-sources)
+
+### Using experimental native DataFusion reader
+Unlike to native Comet reader the Datafusion reader fully supports nested types processing. This reader is currently experimental only
+
+To build Comet with native DataFusion reader and remote HDFS support it is required to have a JDK installed
+
+Example:
+Build a Comet for `spark-3.4` provide a JDK path in `JAVA_HOME`
+Provide the JRE linker path in `RUSTFLAGS`, the path can vary depending on the system. Typically JRE linker is a part of installed JDK
+
+```shell
+export JAVA_HOME="/opt/homebrew/opt/openjdk@11"
Review Comment:
nit: is JAVA_HOME still the requirement?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#issuecomment-2661096890
> oops, @comphead would you mind merging the latest main into this PR branch in order to resolve the conflict?
Resolved, please have another look
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1944107142
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1863,42 @@ fn trim_end(s: &str) -> &str {
}
}
+// Default object store which is local filesystem
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<ObjectStoreUrl, ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
+ 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
Review Comment:
> do you mean the better place to register the object store will be inside file_partitions iterator loop ?
Yes, is it possible that native scan paths correspond to multiple object stores or are different from spark.defaultFs?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1945284761
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1863,42 @@ fn trim_end(s: &str) -> &str {
}
}
+// Default object store which is local filesystem
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<ObjectStoreUrl, ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
+ 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
Review Comment:
for HDFS/S3 the default fs can be taken from `spark.hadoop.fs.defaultFS` parameter.
To support multiple object stores that is interesting idea however I'm not sure when it can be addressed
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1946767459
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1863,42 @@ fn trim_end(s: &str) -> &str {
}
}
+// Default object store which is local filesystem
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<ObjectStoreUrl, ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
+ 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
Review Comment:
that is interesting scenario, I'll add a separate test case for this
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1956991849
##########
native/core/src/execution/planner.rs:
##########
@@ -1155,12 +1154,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))?;
Review Comment:
https://github.com/apache/datafusion-comet/issues/1407. There's no detail in the PR. Can you assign to me if possible, and I'll remember to take care of it.
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936553571
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1864,40 @@ fn trim_end(s: &str) -> &str {
}
}
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<(), ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
Review Comment:
It depends on the feature enabled for the Comet. LocalFileSystem is by default if no specific features selected.
the annotation on this method is
```
#[cfg(not(feature = "hdfs"))]
```
This allows to plugin other features like S3, etc
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1939824736
##########
native/core/src/execution/planner.rs:
##########
@@ -1155,12 +1154,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))?;
Review Comment:
Thats a good point, to verify it we probably need to read Iceberg from HDFS which can be done in #1367
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1956765242
##########
native/core/Cargo.toml:
##########
@@ -77,6 +77,7 @@ datafusion-comet-proto = { workspace = true }
object_store = { workspace = true }
url = { workspace = true }
chrono = { workspace = true }
+datafusion-objectstore-hdfs = { git = "https://github.com/comphead/datafusion-objectstore-hdfs", branch = "master", optional = true }
Review Comment:
Done
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "parthchandra (via GitHub)" <gi...@apache.org>.
parthchandra commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1936547960
##########
native/core/src/parquet/parquet_support.rs:
##########
@@ -1861,6 +1864,40 @@ fn trim_end(s: &str) -> &str {
}
}
+#[cfg(not(feature = "hdfs"))]
+pub(crate) fn register_object_store(
+ session_context: Arc<SessionContext>,
+) -> Result<(), ExecutionError> {
+ let object_store = object_store::local::LocalFileSystem::new();
Review Comment:
It doesn't have to be only a local file system.
##########
native/core/src/execution/planner.rs:
##########
@@ -1220,7 +1217,7 @@ 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 object_store_url = ObjectStoreUrl::parse("hdfs://namenode:9000").unwrap();
Review Comment:
The url should be available as part of the file path passed in. (see line 1178 above)
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#issuecomment-2626183618
## [Codecov](https://app.codecov.io/gh/apache/datafusion-comet/pull/1359?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 57.42%. Comparing base [(`f09f8af`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/f09f8af64c6599255e116a376f4f008f2fd63b43?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`b6c3d2b`)](https://app.codecov.io/gh/apache/datafusion-comet/commit/b6c3d2b57b1674995ccf015fe6e3593ce3ffb202?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> Report is 16 commits behind head on main.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #1359 +/- ##
============================================
+ Coverage 56.12% 57.42% +1.30%
Complexity 976 976
============================================
Files 119 121 +2
Lines 11743 11921 +178
Branches 2251 2253 +2
============================================
+ Hits 6591 6846 +255
+ Misses 4012 3917 -95
- Partials 1140 1158 +18
```
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/datafusion-comet/pull/1359?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on code in PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#discussion_r1939820038
##########
Makefile:
##########
@@ -95,7 +98,7 @@ release-linux: clean
cd native && RUSTFLAGS="-Ctarget-cpu=native -Ctarget-feature=-prefer-256-bit" cargo build --release
./mvnw install -Prelease -DskipTests $(PROFILES)
release:
- cd native && RUSTFLAGS="-Ctarget-cpu=native" cargo build --release
+ cd native && RUSTFLAGS="$(RUSTFLAGS) -Ctarget-cpu=native" && RUSTFLAGS=$$RUSTFLAGS cargo build --release $(FEATURES_ARG)
Review Comment:
Makefile syntax lightly different from bash from what I learned.
so in order to access enviroment variable created on fly it is needed to access is with $$
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: add experimental remote HDFS support for native DataFusion reader [datafusion-comet]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #1359:
URL: https://github.com/apache/datafusion-comet/pull/1359#issuecomment-2629508370
@andygrove @parthchandra @mbutrovich @kazuyukitanimura can I have a review please?
--
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@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org