You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "ted-jenks (via GitHub)" <gi...@apache.org> on 2023/02/07 08:44:29 UTC
[GitHub] [spark] ted-jenks opened a new pull request, #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
ted-jenks opened a new pull request, #39927:
URL: https://github.com/apache/spark/pull/39927
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'core/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
Remove blank line removal from CSVExprUtils. This is currently unused as the parser used to generate inputs for this code-path removes blank and whitespace lines.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
The current state is misleading, indicating that the function is responsisble for functionality that it is not. The multiline codepath does not contain any methods claiming to remove blank lines, leading to a confusing disparity between the two. If in the future blank lines need to be removed from an `Iterator[String]`, this behavior should be added back explicitely.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as the documentation fix.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
No
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
See CSVSuite tests that read "cars..." files which contain blank lines on the multiline and non-multiline code-path.
--
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
[GitHub] [spark] ted-jenks closed pull request #39927: [SPARK-42373][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks closed pull request #39927: [SPARK-42373][SQL] Remove unused blank line removal from CSVExprUtils
URL: https://github.com/apache/spark/pull/39927
--
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
[GitHub] [spark] ted-jenks commented on a diff in pull request #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks commented on code in PR #39927:
URL: https://github.com/apache/spark/pull/39927#discussion_r1098497933
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVUtils.scala:
##########
@@ -29,17 +29,23 @@ object CSVUtils {
* This is currently being used in CSV schema inference.
*/
def filterCommentAndEmpty(lines: Dataset[String], options: CSVOptions): Dataset[String] = {
- // 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.
+ val nonEmptyLines = filterEmpty(lines)
+ filterComment(nonEmptyLines, options)
+ }
+
+ def filterComment(lines: Dataset[String], options: CSVOptions): Dataset[String] = {
+ import lines.sqlContext.implicits._
+ lines.rdd.mapPartitions(
+ CSVExprUtils.filterComment(_, options)).toDS()
+ }
+
+ /**
+ * Filter empty lines from dataset. This is currently being used in CSV schema inference.
+ */
+ private def filterEmpty(lines: Dataset[String]): Dataset[String] = {
Review Comment:
As noted by @HyukjinKwon , blank record skipping is used for `DataFrameReader.csv(dataset: Dataset[String])`
--
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
[GitHub] [spark] ted-jenks commented on a diff in pull request #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks commented on code in PR #39927:
URL: https://github.com/apache/spark/pull/39927#discussion_r1098467684
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVUtils.scala:
##########
@@ -29,17 +29,23 @@ object CSVUtils {
* This is currently being used in CSV schema inference.
*/
def filterCommentAndEmpty(lines: Dataset[String], options: CSVOptions): Dataset[String] = {
- // 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.
Review Comment:
As per this comment, this PR removes the need for repeated logic.
--
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
[GitHub] [spark] HyukjinKwon commented on a diff in pull request #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on code in PR #39927:
URL: https://github.com/apache/spark/pull/39927#discussion_r1098382224
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVExprUtils.scala:
##########
@@ -21,38 +21,34 @@ import org.apache.commons.lang3.StringUtils
object CSVExprUtils {
/**
- * Filter ignorable rows for CSV iterator (lines empty and starting with `comment`).
+ * Filter ignorable rows starting with `comment`.
* This is currently being used in CSV reading path and CSV schema inference.
*/
- def filterCommentAndEmpty(iter: Iterator[String], options: CSVOptions): Iterator[String] = {
+ def filterComment(iter: Iterator[String], options: CSVOptions): Iterator[String] = {
if (options.isCommentSet) {
val commentPrefix = options.comment.toString
- iter.filter { line =>
- line.trim.nonEmpty && !line.startsWith(commentPrefix)
- }
+ iter.dropWhile(_.startsWith(commentPrefix)
} else {
- iter.filter(_.trim.nonEmpty)
Review Comment:
IIRC this matters when you call `DataFrameReader.csv(dataset: Dataset[String])`
--
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
[GitHub] [spark] ted-jenks commented on a diff in pull request #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks commented on code in PR #39927:
URL: https://github.com/apache/spark/pull/39927#discussion_r1098398652
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVExprUtils.scala:
##########
@@ -21,38 +21,34 @@ import org.apache.commons.lang3.StringUtils
object CSVExprUtils {
/**
- * Filter ignorable rows for CSV iterator (lines empty and starting with `comment`).
+ * Filter ignorable rows starting with `comment`.
* This is currently being used in CSV reading path and CSV schema inference.
*/
- def filterCommentAndEmpty(iter: Iterator[String], options: CSVOptions): Iterator[String] = {
+ def filterComment(iter: Iterator[String], options: CSVOptions): Iterator[String] = {
if (options.isCommentSet) {
val commentPrefix = options.comment.toString
- iter.filter { line =>
- line.trim.nonEmpty && !line.startsWith(commentPrefix)
- }
+ iter.dropWhile(_.startsWith(commentPrefix)
} else {
- iter.filter(_.trim.nonEmpty)
Review Comment:
You may be correct. However, it seems the actual read path of `DataFrameReader.csv(dataset: Dataset[String])` goes through 'CSVUtils.filterCommentAndEmpty(lines: Dataset[String], options: CSVOptions)' whereas `TextInputCSVDataSource.inferFromDataset(...)` goes through `CSVUtils.filterCommentAndEmpty(iter: Iterator[String], options: CSVOptions)` (which calls `CSVExprUtils.filterCommentAndEmpty(iter: Iterator[String], options: CSVOptions)`. In any case, I think there is definitely an oportunity to make things a little more clear here and consistent between multiline and non-multiline (though different for `Dataset[String]`) If i figure something out, I'll open a ticket. Will close this PR if not.
--
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
[GitHub] [spark] ted-jenks commented on a diff in pull request #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks commented on code in PR #39927:
URL: https://github.com/apache/spark/pull/39927#discussion_r1098550628
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/csv/CSVExprUtils.scala:
##########
@@ -21,38 +21,34 @@ import org.apache.commons.lang3.StringUtils
object CSVExprUtils {
/**
- * Filter ignorable rows for CSV iterator (lines empty and starting with `comment`).
+ * Filter ignorable rows starting with `comment`.
* This is currently being used in CSV reading path and CSV schema inference.
*/
- def filterCommentAndEmpty(iter: Iterator[String], options: CSVOptions): Iterator[String] = {
+ def filterComment(iter: Iterator[String], options: CSVOptions): Iterator[String] = {
if (options.isCommentSet) {
val commentPrefix = options.comment.toString
- iter.filter { line =>
- line.trim.nonEmpty && !line.startsWith(commentPrefix)
- }
+ iter.dropWhile(_.startsWith(commentPrefix)
} else {
- iter.filter(_.trim.nonEmpty)
Review Comment:
I've addressed this now, things a bit more consistent. Only `CSVUtils` is concerned with the `Dataset[String]` case and the CSV file reads are concerned with `CSVExprUtils`. Would be interested to see what you think of this as I was very confused when trying to do some CSV work to find blank lines were not in fact removed here.
--
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
[GitHub] [spark] HyukjinKwon commented on pull request #39927: [SPARK-42373][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "HyukjinKwon (via GitHub)" <gi...@apache.org>.
HyukjinKwon commented on PR #39927:
URL: https://github.com/apache/spark/pull/39927#issuecomment-1420643952
Doesn't look the refactoring makes it particularly easier to read to me.
--
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
[GitHub] [spark] ted-jenks commented on a diff in pull request #39927: [SPARK-42373][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks commented on code in PR #39927:
URL: https://github.com/apache/spark/pull/39927#discussion_r1098632255
##########
sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/CSVUtils.scala:
##########
@@ -26,20 +26,26 @@ import org.apache.spark.sql.functions._
object CSVUtils {
/**
* Filter ignorable rows for CSV dataset (lines empty and starting with `comment`).
- * This is currently being used in CSV schema inference.
+ * This is currently being used in CSV schema inference and Dataset[String] reading path.
Review Comment:
Was already being used for this but undocumented
--
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
[GitHub] [spark] ted-jenks commented on pull request #39927: [SPARK-42373][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "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
[GitHub] [spark] ted-jenks closed pull request #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks closed pull request #39927: [WIP][SQL] Remove unused blank line removal from CSVExprUtils
URL: https://github.com/apache/spark/pull/39927
--
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
[GitHub] [spark] ted-jenks commented on pull request #39927: [SPARK-42373][SQL] Remove unused blank line removal from CSVExprUtils
Posted by "ted-jenks (via GitHub)" <gi...@apache.org>.
ted-jenks commented on PR #39927:
URL: https://github.com/apache/spark/pull/39927#issuecomment-1429852025
For the main use-case for this, https://github.com/apache/spark/pull/39907, I have settled to define an ordering that doesn't become unclear with these method names.
--
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