[GLUTEN-9152][CORE] Avoid unnecessary serialization of hadoop conf by zml1206 · Pull Request #9153 · apache/gluten

zml1206 · 2025-03-27T06:51:54Z

What changes were proposed in this pull request?

Significantly reduce the closure serialization time of BasicScanExecTransformer and WholeStageTransformer.
(Fixes: #9152)

How was this patch tested?

github-actions · 2025-03-27T06:52:10Z

#9152

github-actions · 2025-03-27T06:52:27Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-03-27T07:54:01Z

Run Gluten Clickhouse CI on x86

zhouyuan · 2025-03-27T20:01:29Z

      rootPaths: Seq[String],
      properties: Map[String, String],
-      serializableHadoopConf: Option[SerializableConfiguration] = None): ValidationResult = {
+      hadoopConf: Configuration): ValidationResult = {


The serialize code is added for viewfs support as we need to pass some info to Velox. Not sure if the new code can be ser/de-ser and passed down.
The code itself is correct, but there is no viewfs related tests in GHA now so we may need to check this manually

Cc @wangyum @JkSelf

But from the code logic, I did not find that velox uses the hadoop conf here.

@wangyum Could you please help in verifying this PR in your ViewFS environment? My ViewFS test setup is currently not functioning properly.

OK. will test it soon.

I have tested this patch on our cluster and it still works.

Yohahaha · 2025-03-28T03:25:13Z


  val sparkConf: SparkConf = sparkContext.getConf

-  val serializableHadoopConf: SerializableConfiguration = new SerializableConfiguration(


maybe add @transient lazy could fix the issue

If serialization is require, lazy is a solution to avoid serialization without enabling viewFS, but if serialization is not needed here, it would be simpler to use sparkContext.hadoopConfiguration directly.

philo-he

Thanks!

github-actions · 2025-03-29T00:15:23Z

Run Gluten Clickhouse CI on x86

JkSelf · 2025-03-31T00:42:04Z

@zml1206 can you help to take a look the failed unit tests?

zml1206 · 2025-03-31T01:33:27Z

@zml1206 can you help to take a look the failed unit tests?

It's strange that PR seems to cause Java heap oom, but JDK 17 passes. I haven't found the reason yet. I'm not sure whether it is related to PR or the test itself is unstable.

github-actions · 2025-04-01T09:42:34Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-04-01T10:27:42Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-04-01T13:54:29Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-04-02T06:11:00Z

Run Gluten Clickhouse CI on x86

github-actions · 2025-04-08T04:01:57Z

Run Gluten Clickhouse CI on x86

zml1206 · 2025-04-08T06:08:08Z

I can't reproduce it locally. I tried many ways, including lazy, @transient, local variables, etc. WholeStageTransformer in spark 3.5 and jdk8 will cause OOM in HashExpressionSuite. I guess it is related to jdk8's GC. Now there is no spark 3.5-jdk8 workflow. Can it be merged or roll back the changes of WholeStageTransformer? @JkSelf

wangyum · 2025-04-08T06:20:38Z

@zml1206 Can we increase JVM heap here, e.g.:

<argLine>${argLine} -Xmx4g ${extraJavaTestArgs}</argLine>

zml1206 · 2025-04-08T07:00:00Z

@zml1206 Can we increase JVM heap here, e.g.:
<argLine>${argLine} -Xmx4g ${extraJavaTestArgs}</argLine>

Thank you, after #9209, there is no CI for spark3.5-jdk8.

philo-he · 2025-04-08T08:03:10Z

I can't reproduce it locally. I tried many ways, including lazy, @transient, local variables, etc. WholeStageTransformer in spark 3.5 and jdk8 will cause OOM in HashExpressionSuite. I guess it is related to jdk8's GC. Now there is no spark 3.5-jdk8 workflow. Can it be merged or roll back the changes of WholeStageTransformer? @JkSelf

@zml1206, do you mean reverting the changes for WholeStageTransformer can make that test pass in the previous CI test? If yes, it's strange since that code path is for viewFS and viewFS is not verified in CI.

zml1206 · 2025-04-08T08:05:57Z

I can't reproduce it locally. I tried many ways, including lazy, @transient, local variables, etc. WholeStageTransformer in spark 3.5 and jdk8 will cause OOM in HashExpressionSuite. I guess it is related to jdk8's GC. Now there is no spark 3.5-jdk8 workflow. Can it be merged or roll back the changes of WholeStageTransformer? @JkSelf

@zml1206, do you mean reverting the changes for WholeStageTransformer can make that test pass in the previous CI test? If yes, it's strange since that code path is for viewFS and viewFS is not verified in CI.

Yes, revert the changes for WholeStageTransformer can make that test pass in the previous CI test.

philo-he

I think we can merge this patch first. If there is some issue found at user side, we can make a follow-up pr.

zml1206 · 2025-04-08T08:57:44Z

Thanks for everyone's review, if no one else has any comments, I'll merge it later.

[GLUTEN-9152][CORE] Avoid unnecessary serialization of hadoop conf

63a1346

github-actions bot added CORE works for Gluten Core VELOX CLICKHOUSE labels Mar 27, 2025

fix

ed3035a

zml1206 requested review from JkSelf and philo-he March 27, 2025 09:48

zhouyuan reviewed Mar 27, 2025

View reviewed changes

Yohahaha reviewed Mar 28, 2025

View reviewed changes

philo-he approved these changes Mar 28, 2025

View reviewed changes

wangyum approved these changes Mar 28, 2025

View reviewed changes

Merge branch 'main' into 9152

6d423c9

temporary solution for oom

9147532

fix

fab6b38

fix

cfc64a0

fix

41904be

zml1206 marked this pull request as draft April 3, 2025 22:22

zml1206 added 2 commits April 8, 2025 11:58

Merge branch 'main' into 9152

dd31d09

fix

d6c6d0c

zml1206 marked this pull request as ready for review April 8, 2025 06:07

philo-he approved these changes Apr 8, 2025

View reviewed changes

zml1206 merged commit d27959c into apache:main Apr 8, 2025
47 checks passed

zml1206 deleted the 9152 branch December 9, 2025 08:14


		val sparkConf: SparkConf = sparkContext.getConf

		val serializableHadoopConf: SerializableConfiguration = new SerializableConfiguration(

Conversation

zml1206 commented Mar 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

How was this patch tested?

Uh oh!

github-actions bot commented Mar 27, 2025

Uh oh!

github-actions bot commented Mar 27, 2025

Uh oh!

github-actions bot commented Mar 27, 2025

Uh oh!

zhouyuan Mar 27, 2025

Choose a reason for hiding this comment

Uh oh!

zml1206 Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

JkSelf Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

wangyum Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

wangyum Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Yohahaha Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

zml1206 Mar 28, 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!

philo-he left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 29, 2025

Uh oh!

JkSelf commented Mar 31, 2025

Uh oh!

zml1206 commented Mar 31, 2025

Uh oh!

github-actions bot commented Apr 1, 2025

Uh oh!

github-actions bot commented Apr 1, 2025

Uh oh!

github-actions bot commented Apr 1, 2025

Uh oh!

github-actions bot commented Apr 2, 2025

Uh oh!

github-actions bot commented Apr 8, 2025

Uh oh!

zml1206 commented Apr 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wangyum commented Apr 8, 2025

Uh oh!

zml1206 commented Apr 8, 2025

Uh oh!

philo-he commented Apr 8, 2025

Uh oh!

zml1206 commented Apr 8, 2025

Uh oh!

philo-he left a comment

Choose a reason for hiding this comment

Uh oh!

zml1206 commented Apr 8, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zml1206 commented Mar 27, 2025 •

edited

Loading

zml1206 Mar 28, 2025 •

edited

Loading

zml1206 commented Apr 8, 2025 •

edited

Loading