JoshRosen commented on PR #46894: URL: https://github.com/apache/spark/pull/46894#issuecomment-2154144411
> > Do we have any unit tests for escapePathName? > > https://github.com/apache/spark/blob/f9542d008402f8cef96d5ec347583c7c1d30d840/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLSuite.scala#L1803 Although it's true that those tests indirectly cover these paths, by "unit test" I was referring to a dedicated test of just `escapePathString` itself which actually asserts over the form of the output. As it stands now, the test in DDLSuite is asserting over some end-to-end functionality to check that the path escaping logic is actually invoked, but that test would not fail if we made breaking or incompatible changes to the escaping logic itself. For example, if we switched from uppercase to lowercase hexadecimal conversions then that change wouldn't cause the DDLSuite test to fail. In addition, the more "end-to-end" tests are slow to run. I think it can still be useful to have relatively more end-to-end tests covering important scenarios (since special characters in paths could cause problems at other layers and we might want more end-to-end coverage for such potential cases), but I think we should augment those tests with _dedicated_ unit tests for the helper functions to assert that their actual output doesn't change. This will be useful if we choose to optimize the int -> hex conversion (which I'll suggest in another comment). -- 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]
