ted-jenks commented on PR #39927: URL: https://github.com/apache/spark/pull/39927#issuecomment-1420734577
> Doesn't look the refactoring makes it particularly easier to read to me. @HyukjinKwon I was doing some work towards another issue [here](https://github.com/apache/spark/pull/39907/files?diff=unified&w=0#diff-1be457ec2ce75e3c193a34047b9ed774399d7f5710411eb96b20157392868e93) that I think demonstrates the problem. The code reads: ``` val skippedLines = iter.drop(options.skipLines) filterCommentAndEmpty(skippedLines, options) ``` It looks like: 1. `options.skipLines` lines are dropped 2. comments and blanks are removed What actually happens: 1. blank lines are removed 2. `options.skipLines` lines are dropped 3. comments and blanks are removed This refactoring would make it clearer and consistent with the multiline case. It would then look like: ``` val skippedLines = iter.drop(options.skipLines) filterComment(skippedLines, options) ``` and behave as expected when reading the code. Additionally, this refactoring removes the function introduced in SPARK-18362 that is supposed to be removed as per the doc: ``` // Note that this was separately made by SPARK-18362. Logically, this should be the same // with the one below, `filterCommentAndEmpty` but execution path is different. One of them // might have to be removed in the near future if possible. ``` -- 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]
