sarutak commented on PR #59125: URL: https://github.com/apache/spark/pull/59125#issuecomment-6190310636
Hi @vboo123, before merging, could you address two small things? 1. The PR description is out of date. "How was this patch tested?" still describes a single test, while the PR now has three (COUNT, GROUP BY over a null partition, and multiple partition columns). Also, "it runs for Parquet and ORC across V1 and V2" reads as if both paths verify the fix, but aggregate push down only happens on the DSv2 file source, so the fix is exercised only by the V2 suites. Since the description becomes the commit message, it would be good to list the three tests and note that the push down checks are effective only on V2. Optionally, the user-facing change section could also mention that the issue only shows up when the DSv2 file source is used, since parquet and orc are in spark.sql.sources.useV1SourceList by default. 2. In the GROUP BY test, the comment says the query "routes a NULL group key through the reOrderPartitionCol/JoinedRow path". With a single partition column, reOrderPartitionCol returns partitionValues as is (partitionSchema.names.sameElements(groupByColNames) is true), so only the JoinedRow part is exercised. This wording came from my earlier suggestion, so that one is on me. Something like "through the JoinedRow (group key + footer aggregate) path" would be accurate. My earlier note about the V1 suites stands as is: no test change is needed for that, and item 1 is only about the descrip\ tion wording. -- 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. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
