sririshindra commented on issue #28173: [SPARK-31389][SQL][TESTS] Ensure all tests in SQLMetricsSuite run with both codegen on and off URL: https://github.com/apache/spark/pull/28173#issuecomment-615997103 > Thank you for your contribution, @sririshindra . BTW, the PR title `Ensure all tests` sounds misleading to me because > > 1. This PR doesn't run codegen on/off path in many test cases like `test("save metrics")`. > 2. This PR doesn't change `test("Aggregate metrics")` although it has `testSparkPlanMetrics`. > > IIUC, this PR seems to aim to change all test cases having `testSparkPlanMetrics`. Did I understand correctly? @dongjoon-hyun Thanks for your comments. You are correct that the title of the PR is misleading. 1. As you said, my intention was to change all test cases having testSparkPlanMetrics and add codegen counterparts to those tests. 2. The PR doesn't change `test("Aggregate metrics")` and `test("ObjectHashAggregate metrics")`. - The `test("Aggregate metrics")` tests metrics when a HashAggregate is used. Enabling codegen forces the test to use ObjectHashAggregate rather than the regular HashAggregate. ObjectHashAggregate has a test of its own. Therefore, I feel these two tests need not enabling codegen is not necessary. Please let me know what do you think.
---------------------------------------------------------------- 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. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
