[SPARK-32851][SQL][TEST] Tests should fail if errors happen when generating projection code #29721
Conversation
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala
Outdated
Show resolved
Hide resolved
|
Test build #128547 has finished for PR 29721 at commit
|
|
Test build #128555 has finished for PR 29721 at commit
|
|
GA passed. cc: @cloud-fan @viirya |
| private lazy val proj = FailedCodegenProjection.createObject(child :: Nil) | ||
| override def dataType: DataType = LongType | ||
| override def eval(input: InternalRow): Any = { | ||
| proj(input).getLong(0) |
There was a problem hiding this comment.
This test proves that if eval fails, the query fails. I think we should override doCodeGen and produce invalid code there?
There was a problem hiding this comment.
This test proves that, if the codegen of proj fails, the query fails. If CODEGEN_FACTORY_MODE=fallback, proj falls back into an interpreted one.
There was a problem hiding this comment.
Then shall we just add a test in CodeGeneratorWithInterpretedFallbackSuite, to make sure that FailedCodegenProjection fails with default codegen mode?
There was a problem hiding this comment.
My point is that, the test here is not really an end-to-end test. It's just used to trigger FailedCodegenProjection
There was a problem hiding this comment.
It's just used to trigger FailedCodegenProjection
Yea, yes. This test just triggers the the codegen failure of FailedCodegenProjection in executor sides and it checks the failure correctly propagates to a driver side in SharedSparkSession by setting the config CODEGEN_FACTORY_MODE=codegen_only in SparkConf:
https://github.com/apache/spark/pull/29721/files#diff-ef6933b96996929c402e3a36296b2590R71
CodeGeneratorWithInterpretedFallbackSuite is placed in the catalyst package, so it is not related to adding the config in SharedSparkSession. But, if the existing tests in CodeGeneratorWithInterpretedFallbackSuite look enough, I think it is okay to drop the added tests and just add the config CODEGEN_FACTORY_MODE in SharedSparkSession.
There was a problem hiding this comment.
Yea I think it's OK to just change the config in test.
maropu
changed the title
|
Test build #128571 has finished for PR 29721 at commit
|
|
Thanks! Merged to master. |
5 participants
What changes were proposed in this pull request?
This PR intends to set
CODEGEN_ONLYatCODEGEN_FACTORY_MODEin test spark context so that tests can fail if errors happen when generating expr code.Why are the changes needed?
I noticed that the code generation of
SafeProjectionfailed in the existing test (https://issues.apache.org/jira/browse/SPARK-32828) but it passed becauseFALLBACKwas set atCODEGEN_FACTORY_MODE(by default) inSharedSparkSession. To get aware of these failures quickly, I think its worth settingCODEGEN_ONLYatCODEGEN_FACTORY_MODE.Does this PR introduce any user-facing change?
No.
How was this patch tested?
Existing tests.