Skip to content
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

[SPARK-51046][SQL][TEST] Reduce numCols in withFilter() to prevent SubExprEliminationBenchmark from failing due to a Codegen error #49938

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

wayneguow
Copy link
Contributor

What changes were proposed in this pull request?

This PR aims to reduce numCols in withFilter() to prevent SubExprEliminationBenchmark from failing due to a Codegen error.

I did some debug investigation and found that in the current master branch, the codegen code has a huge processNext method, but in branch-3.5, it is split into many small methods. Because the logic of codegen is different, the previous benchmark cannot run normally.

master branch:

protected void processNext() throws java.io.IOException {

	while ( inputadapter_input_0.hasNext()) {
                 ...
        }
}

3.5 branch:

        public boolean eval(InternalRow i) {
		boolean value_2997 = Or_498(i);
		return !globalIsNull_498 && value_2997;
	}
        private boolean Or_498(InternalRow i) {
                ...
        }
        ...
        private boolean Or_xxxx(InternalRow i) {
               ...
        }    

Why are the changes needed?

If we run SubExprEliminationBenchmark:

build/sbt "sql/Test/runMain org.apache.spark.sql.execution.SubExprEliminationBenchmark"

It fails at CodeGenerator, details:

[info] Running benchmark: from_json as subExpr in Filter
[info] Running case: subExprElimination false, codegen: true
[info] 00:08:31.509 ERROR org.apache.spark.sql.catalyst.expressions.codegen.CodeGenerator: Failed to compile the generated Java code.
[info] org.codehaus.commons.compiler.InternalCompilerException: Compiling "GeneratedClass" in File 'generated.java', Line 1, Column 1: File 'generated.java', Line 24, Column 16: Compiling "processNext()"

The root cause is:

[error] Caused by: org.codehaus.commons.compiler.InternalCompilerException: Code grows beyond 64 KB
[error] at org.codehaus.janino.CodeContext.makeSpace(CodeContext.java:699)
[error] at org.codehaus.janino.CodeContext.write(CodeContext.java:558)
[error] at org.codehaus.janino.UnitCompiler.write(UnitCompiler.java:13079)
[error] at org.codehaus.janino.UnitCompiler.store(UnitCompiler.java:12752)
[error] at org.codehaus.janino.UnitCompiler.store(UnitCompiler.java:12730)
[error] at org.codehaus.janino.UnitCompiler.compile2(UnitCompiler.java:2742)
[error] ... 164 more

Does this PR introduce any user-facing change?

No, just fix a test benchmark failure.

How was this patch tested?

Run benchmark tests manually.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the SQL label Feb 13, 2025
@wayneguow
Copy link
Contributor Author

subExprElimination true, codegen: false 2053 2079 33 0.0 20526629.8 3.5X
subExprElimination false, codegen: true 2474 2512 44 0.0 24744107.3 1.0X
subExprElimination false, codegen: false 2231 2246 20 0.0 22306061.2 1.1X
subExprElimination true, codegen: true 2408 2509 100 0.0 24084091.2 1.0X
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What confuses me is that when subExprElimination set to true, codegen set to true, there is an obvious regression in performance compared to before?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's true. The performance regression was the actual root cause why we didn't make a decision. Here, FYI.

@wayneguow
Copy link
Contributor Author

cc @dongjoon-hyun , Could you take a look when you have some time? maybe we can have some discussion.

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Feb 13, 2025

cc @panbingkun , @cloud-fan , @LuciferYang

It seems that we need to make a decision. Are we good with this codeine perf regression of from_json?

@dongjoon-hyun
Copy link
Member

BTW, Thank you for your active contributions, @wayneguow !

@wayneguow
Copy link
Contributor Author

BTW, Thank you for your active contributions, @wayneguow !

Happy to contribute!😀 Keep going!

@LuciferYang
Copy link
Contributor

cc @panbingkun , @cloud-fan , @LuciferYang

It seems that we need to make a decision. Are we good with this codeine perf regression of from_json?

@panbingkun Can #49573 be completed before the 4.0 release? If so, we can wait until the optimization is finished before refreshing this result. Additionally, if #49573 is completed, will we still need to change numCols from 500 to 330?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants