[GitHub] [spark] sririshindra edited a comment on issue #28173: [SPARK-31389][SQL][TESTS] Add codegen-on test coverage for some tests in SQLMetricsSuite

[GitBox <gi...@apache.org>]

sririshindra edited a comment on issue #28173:
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 enable codegen. 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:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org