[GitHub] [spark] ted-jenks commented on pull request #39927: [SPARK-42373][SQL] Remove unused blank line removal from CSVExprUtils

["ted-jenks (via GitHub)" <gi...@apache.org>]

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: reviews-unsubscribe@spark.apache.org

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