chore: Add hdfs feature test job #2350

wForget · 2025-09-09T03:06:35Z

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

How are these changes tested?

codecov-commenter · 2025-09-09T03:28:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 57.42%. Comparing base (f09f8af) to head (456a24a).
⚠️ Report is 496 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #2350      +/-   ##
============================================
+ Coverage     56.12%   57.42%   +1.29%     
- Complexity      976     1297     +321     
============================================
  Files           119      147      +28     
  Lines         11743    13415    +1672     
  Branches       2251     2347      +96     
============================================
+ Hits           6591     7703    +1112     
- Misses         4012     4451     +439     
- Partials       1140     1261     +121

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

wForget · 2025-09-11T09:11:43Z

.github/workflows/pr_build_linux.yml

+          artifact_name: ${{ matrix.os }}-java-${{ matrix.java_version }}-features-${{ matrix.features.value }}-${{ github.run_id }}-${{ github.run_number }}-${{ github.run_attempt }}
+          features: ${{ matrix.features.value }}
+          maven_opts: "-Dtest=none -Dfeatures=${{ matrix.features.value }}"


-Dtest=none skip java tests

wForget · 2025-09-11T09:13:34Z

spark/src/test/scala/org/apache/spark/sql/CometTestBase.scala

@@ -1138,4 +1138,9 @@ abstract class CometTestBase
    usingDataSourceExec(conf) &&
    !CometConf.COMET_SCAN_ALLOW_INCOMPATIBLE.get(conf)
  }
+
+  def featureEnabled(feature: String): Boolean = {
+    System.getProperty("feature", "").split(",").contains(feature)


A temporary solution before #2372

wForget · 2025-09-11T10:16:19Z

@parthchandra Could you please take a look?

comphead · 2025-09-11T15:17:07Z

spark/src/test/scala/org/apache/comet/parquet/ParquetReadFromHdfsSuite.scala

+      {
+        val testFilePath = dir.toString
+        writeTestParquetFile(testFilePath)
+        withSQLConf(CometConf.COMET_NATIVE_SCAN_IMPL.key -> CometConf.SCAN_NATIVE_DATAFUSION) {


IMO we also should check native_iceberg_compat`

Thanks, I will add native_iceberg_compat mode

comphead · 2025-09-11T15:18:35Z

spark/src/test/scala/org/apache/comet/parquet/ParquetReadFromFakeHadoopFsSuite.scala

@@ -74,7 +74,9 @@ class ParquetReadFromFakeHadoopFsSuite extends CometTestBase with AdaptiveSparkP
        .startsWith(FakeHDFSFileSystem.PREFIX))
  }

-  ignore("test native_datafusion scan on fake fs") {
+  test("test native_datafusion scan on fake fs") {


should we also test hdfs feature and native_iceberg_compat mode?

should we also test hdfs feature

hdfs feature does not seem to support non-hdfs scheme yet, as comment #2360 (comment) .

datafusion-comet/native/fs-hdfs/src/hdfs.rs

Lines 844 to 865 in 72eb0e9

fn get_namenode_uri(path: &str) -> Result<String, HdfsErr> {

match Url::parse(path) {

Ok(url) => match url.scheme() {

LOCAL_FS_SCHEME => Ok("file:///".to_string()),

HDFS_FS_SCHEME | VIEW_FS_SCHEME => {

if let Some(host) = url.host() {

let mut uri_builder = String::new();

write!(&mut uri_builder, "{}://{}", url.scheme(), host).unwrap();

if let Some(port) = url.port() {

write!(&mut uri_builder, ":{port}").unwrap();

}

Ok(uri_builder)

} else {

Err(HdfsErr::InvalidUrl(path.to_string()))

}

}

_ => Err(HdfsErr::InvalidUrl(path.to_string())),

},

Err(_) => Err(HdfsErr::InvalidUrl(path.to_string())),

}

}

Thats correct, yes

That's one of the fixes we wanted in fs-hdfs datafusion-contrib/fs-hdfs#29

Also, I think at some point it might be more reasonable to use openDAL instead of fs-hdfs (#2367)

parthchandra

Thanks for this PR @wForget. Should we wait to merge #2327 before we merge this?

parthchandra · 2025-09-12T00:43:45Z

spark/src/test/scala/org/apache/comet/parquet/ParquetReadFromFakeHadoopFsSuite.scala

@@ -74,7 +74,9 @@ class ParquetReadFromFakeHadoopFsSuite extends CometTestBase with AdaptiveSparkP
        .startsWith(FakeHDFSFileSystem.PREFIX))
  }

-  ignore("test native_datafusion scan on fake fs") {
+  test("test native_datafusion scan on fake fs") {


Also, I think at some point it might be more reasonable to use openDAL instead of fs-hdfs (#2367)

wForget · 2025-09-12T02:51:48Z

Thanks for this PR @wForget. Should we wait to merge #2327 before we merge this?

Thanks for the review. Do you mean #2372? If so, I think either way is fine — the current temporary solution won’t break CI.

parthchandra · 2025-09-13T00:49:07Z

oops. Yes, I did mean #2372. Let's get that merged and update the test in this PR.

parthchandra · 2025-09-13T00:56:46Z

Should we also be adding the test suites to pr_build_macos.yml ?

wForget · 2025-09-13T01:05:04Z

Should we also be adding the test suites to pr_build_macos.yml ?

Thanks, I will add it

wForget force-pushed the COMET-2298 branch 2 times, most recently from eefa69f to 1aa1d47 Compare September 9, 2025 10:10

wForget mentioned this pull request Sep 11, 2025

Enable feature specific unit tests #2360

Open

wForget marked this pull request as ready for review September 11, 2025 08:01

wForget added 11 commits September 11, 2025 16:09

chore: Add hdfs feature test job

8c1a152

fix and add hdfs suite

d4a9f26

skip java test

0e2fba7

fix

3f94e6b

disable hdrs async_file feature

0ee6cbb

wait for hdfs async blocking thread exit

332dc59

revert disable hdrs async_file

84e4aca

fix

f747615

clean tmp dir

0c062bc

fix comment

162914f

add hdfs test

348342d

wForget force-pushed the COMET-2298 branch from d197150 to 348342d Compare September 11, 2025 08:10

fix

8d219c9

wForget commented Sep 11, 2025

View reviewed changes

comphead reviewed Sep 11, 2025

View reviewed changes

parthchandra reviewed Sep 12, 2025

View reviewed changes

add native_iceberg_compat test

456a24a

	fn get_namenode_uri(path: &str) -> Result<String, HdfsErr> {
	match Url::parse(path) {
	Ok(url) => match url.scheme() {
	LOCAL_FS_SCHEME => Ok("file:///".to_string()),
	HDFS_FS_SCHEME \| VIEW_FS_SCHEME => {
	if let Some(host) = url.host() {
	let mut uri_builder = String::new();
	write!(&mut uri_builder, "{}://{}", url.scheme(), host).unwrap();

	if let Some(port) = url.port() {
	write!(&mut uri_builder, ":{port}").unwrap();
	}
	Ok(uri_builder)
	} else {
	Err(HdfsErr::InvalidUrl(path.to_string()))
	}
	}
	_ => Err(HdfsErr::InvalidUrl(path.to_string())),
	},
	Err(_) => Err(HdfsErr::InvalidUrl(path.to_string())),
	}
	}

chore: Add hdfs feature test job #2350

Are you sure you want to change the base?

chore: Add hdfs feature test job #2350

Conversation

wForget commented Sep 9, 2025

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Uh oh!

codecov-commenter commented Sep 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wForget commented Sep 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wForget Sep 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthchandra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wForget commented Sep 12, 2025

Uh oh!

parthchandra commented Sep 13, 2025

Uh oh!

parthchandra commented Sep 13, 2025

Uh oh!

wForget commented Sep 13, 2025

Uh oh!

Uh oh!

codecov-commenter commented Sep 9, 2025 •

edited

Loading

wForget Sep 11, 2025 •

edited

Loading