You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by HyukjinKwon <gi...@git.apache.org> on 2017/03/02 17:47:58 UTC
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
GitHub user HyukjinKwon opened a pull request:
https://github.com/apache/spark/pull/17142
[SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV parser and minor cleanup
## What changes were proposed in this pull request?
This PR suggests adding some comments in `UnivocityParser` logics to explain what happens. Also, it proposes, IMHO, a little bit cleaner (at least easy for me to explain).
## How was this patch tested?
Unit tests in `CSVSuite`.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/HyukjinKwon/spark SPARK-18699
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/17142.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #17142
----
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17142
@HyukjinKwon you can address @maropu 's comments in your next CSV PR.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104107046
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
+ // 1,2,A
+ //
+ // Also, let's say `columnNameOfCorruptRecord` is set to "_unparsed", `header` is `true`
+ // by user and the user selects "c", "b", "_unparsed" and "a" fields. In this case, we need
+ // to map those values below:
+ //
+ // required schema - ["c", "b", "_unparsed", "a"]
+ // CSV data schema - ["a", "b", "c"]
+ // required CSV data schema - ["c", "b", "a"]
+ //
+ // with the input tokens,
+ //
+ // input tokens - [1, 2, "A"]
+ //
+ // Each input token is placed in each output row's position by mapping these. In this case,
+ //
+ // output row - ["A", 2, null, 1]
+ //
+ // In more details,
+ // - `valueConverters`, input tokens - CSV data schema
+ // `valueConverters` keeps the positions of input token indices (by its index) to each
+ // value's converter (by its value) in an order of CSV data schema. In this case,
+ // [string->int, string->int, string->string].
+ //
+ // - `tokenIndexArr`, input tokens - required CSV data schema
+ // `tokenIndexArr` keeps the positions of input token indices (by its index) to reordered
+ // fields given the required CSV data schema (by its value). In this case, [2, 1, 0].
+ //
+ // - `rowIndexArr`, input tokens - required schema
--- End diff --
ditto
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104104154
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
+ // 1,2,A
+ //
+ // Also, let's say `columnNameOfCorruptRecord` is set to "_unparsed", `header` is `true`
+ // by user and the user selects "c", "b", "_unparsed" and "a" fields. In this case, we need
+ // to map those values below:
+ //
+ // required schema - ["c", "b", "_unparsed", "a"]
+ // CSV data schema - ["a", "b", "c"]
+ // required CSV data schema - ["c", "b", "a"]
+ //
+ // with the input tokens,
+ //
+ // input tokens - [1, 2, "A"]
+ //
+ // Each input token is placed in each output row's position by mapping these. In this case,
+ //
+ // output row - ["A", 2, null, 1]
+ //
+ // In more details,
+ // - `valueConverters`, input tokens - CSV data schema
+ // `valueConverters` keeps the positions of input token indices (by its index) to each
+ // value's converter (by its value) in an order of CSV data schema. In this case,
+ // [string->int, string->int, string->string].
+ //
+ // - `tokenIndexArr`, input tokens - required CSV data schema
+ // `tokenIndexArr` keeps the positions of input token indices (by its index) to reordered
+ // fields given the required CSV data schema (by its value). In this case, [2, 1, 0].
+ //
+ // - `rowIndexArr`, input tokens - required schema
+ // `rowIndexArr` keeps the positions of input token indices (by its index) to reordered
+ // field indices given the required schema (by its value). In this case, [0, 1, 3].
+ private val valueConverters: Array[ValueConverter] =
+ dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
+
+ // Only used to create both `tokenIndexArr` and `rowIndexArr`. This variable means
+ // the fields that we should try to convert.
+ private val reorderedFields = if (options.dropMalformed) {
+ // If `dropMalformed` is enabled, then it needs to parse all the values
+ // so that we can decide which row is malformed.
+ requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
+ } else {
+ requiredSchema
+ }
+
+ private val tokenIndexArr: Array[Int] = {
--- End diff --
How about `fromIndexInTokens` instead of `tokenIndexArr ` for self-describing more?
Along with his, `rowIndexArr` to `toIndexInRow`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104062715
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
--- End diff --
Thanks for cc'ing and brushing-up code ;) I'll check in hours
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/17142
thanks, merging to master!
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104105767
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
+ // 1,2,A
+ //
+ // Also, let's say `columnNameOfCorruptRecord` is set to "_unparsed", `header` is `true`
+ // by user and the user selects "c", "b", "_unparsed" and "a" fields. In this case, we need
+ // to map those values below:
+ //
+ // required schema - ["c", "b", "_unparsed", "a"]
+ // CSV data schema - ["a", "b", "c"]
+ // required CSV data schema - ["c", "b", "a"]
--- End diff --
I feel "required CSV data schema" is a little ambiguous because there is no schema variable along this name in this class. So, it seems we need to describe more?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/17142
except for minor comments, LGTM.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104063545
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
--- End diff --
It is just a little bit for codes.. actually :). I hope the comment makes reading this code easier and not look too verbose.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17142
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/73783/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104106202
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
+ // 1,2,A
+ //
+ // Also, let's say `columnNameOfCorruptRecord` is set to "_unparsed", `header` is `true`
+ // by user and the user selects "c", "b", "_unparsed" and "a" fields. In this case, we need
+ // to map those values below:
+ //
+ // required schema - ["c", "b", "_unparsed", "a"]
+ // CSV data schema - ["a", "b", "c"]
+ // required CSV data schema - ["c", "b", "a"]
+ //
+ // with the input tokens,
+ //
+ // input tokens - [1, 2, "A"]
+ //
+ // Each input token is placed in each output row's position by mapping these. In this case,
+ //
+ // output row - ["A", 2, null, 1]
+ //
+ // In more details,
+ // - `valueConverters`, input tokens - CSV data schema
+ // `valueConverters` keeps the positions of input token indices (by its index) to each
+ // value's converter (by its value) in an order of CSV data schema. In this case,
+ // [string->int, string->int, string->string].
+ //
+ // - `tokenIndexArr`, input tokens - required CSV data schema
+ // `tokenIndexArr` keeps the positions of input token indices (by its index) to reordered
+ // fields given the required CSV data schema (by its value). In this case, [2, 1, 0].
+ //
+ // - `rowIndexArr`, input tokens - required schema
+ // `rowIndexArr` keeps the positions of input token indices (by its index) to reordered
+ // field indices given the required schema (by its value). In this case, [0, 1, 3].
+ private val valueConverters: Array[ValueConverter] =
+ dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
+
+ // Only used to create both `tokenIndexArr` and `rowIndexArr`. This variable means
+ // the fields that we should try to convert.
+ private val reorderedFields = if (options.dropMalformed) {
--- End diff --
`requiredFields` is better?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on the issue:
https://github.com/apache/spark/pull/17142
I definitely will. Thank you so much @cloud-fan and @maropu.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r103986635
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
--- End diff --
cc @maropu and @cloud-fan, could you check if this comment look nicer to you?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/17142
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104103858
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
+ // 1,2,A
+ //
+ // Also, let's say `columnNameOfCorruptRecord` is set to "_unparsed", `header` is `true`
+ // by user and the user selects "c", "b", "_unparsed" and "a" fields. In this case, we need
+ // to map those values below:
+ //
+ // required schema - ["c", "b", "_unparsed", "a"]
+ // CSV data schema - ["a", "b", "c"]
+ // required CSV data schema - ["c", "b", "a"]
+ //
+ // with the input tokens,
+ //
+ // input tokens - [1, 2, "A"]
+ //
+ // Each input token is placed in each output row's position by mapping these. In this case,
+ //
+ // output row - ["A", 2, null, 1]
+ //
+ // In more details,
+ // - `valueConverters`, input tokens - CSV data schema
+ // `valueConverters` keeps the positions of input token indices (by its index) to each
+ // value's converter (by its value) in an order of CSV data schema. In this case,
+ // [string->int, string->int, string->string].
+ //
+ // - `tokenIndexArr`, input tokens - required CSV data schema
+ // `tokenIndexArr` keeps the positions of input token indices (by its index) to reordered
+ // fields given the required CSV data schema (by its value). In this case, [2, 1, 0].
+ //
+ // - `rowIndexArr`, input tokens - required schema
+ // `rowIndexArr` keeps the positions of input token indices (by its index) to reordered
+ // field indices given the required schema (by its value). In this case, [0, 1, 3].
+ private val valueConverters: Array[ValueConverter] =
+ dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
+
+ // Only used to create both `tokenIndexArr` and `rowIndexArr`. This variable means
+ // the fields that we should try to convert.
+ private val reorderedFields = if (options.dropMalformed) {
+ // If `dropMalformed` is enabled, then it needs to parse all the values
+ // so that we can decide which row is malformed.
+ requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
+ } else {
+ requiredSchema
+ }
+
+ private val tokenIndexArr: Array[Int] = {
+ reorderedFields
+ .filter(_.name != options.columnNameOfCorruptRecord)
+ .map(f => dataSchema.indexOf(f)).toArray
+ }
+
+ private val rowIndexArr: Array[Int] = if (corruptFieldIndex.isDefined) {
+ val corrFieldIndex = corruptFieldIndex.get
+ reorderedFields.indices.filter(_ != corrFieldIndex).toArray
+ } else {
+ reorderedFields.indices.toArray
--- End diff --
The code below is better?
```
private val rowIndexArr: Array[Int] = corruptFieldIndex.map { corrFieldIndex =>
reorderedFields.indices.filter(_ != corrFieldIndex).toArray
}.getOrElse {
reorderedFields.indices.toArray
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/17142
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104105409
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
+ // 1,2,A
+ //
+ // Also, let's say `columnNameOfCorruptRecord` is set to "_unparsed", `header` is `true`
+ // by user and the user selects "c", "b", "_unparsed" and "a" fields. In this case, we need
+ // to map those values below:
+ //
+ // required schema - ["c", "b", "_unparsed", "a"]
+ // CSV data schema - ["a", "b", "c"]
--- End diff --
ISTM it'd be better to map the names into these variables here, `reuiqredSchema` and `dataSchema`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17142
**[Test build #73783 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/73783/testReport)** for PR 17142 at commit [`ee30cc3`](https://github.com/apache/spark/commit/ee30cc3a8445ebe62118aa9f69c3306d64b1b4a2).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104104893
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
--- End diff --
How about "_c0,_c1,_c2" in the header line? I couldn't first tell this is a header.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in C...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/17142#discussion_r104107028
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/datasources/csv/UnivocityParser.scala ---
@@ -54,39 +54,77 @@ private[csv] class UnivocityParser(
private val dataSchema = StructType(schema.filter(_.name != options.columnNameOfCorruptRecord))
- private val valueConverters =
- dataSchema.map(f => makeConverter(f.name, f.dataType, f.nullable, options)).toArray
-
private val tokenizer = new CsvParser(options.asParserSettings)
private var numMalformedRecords = 0
private val row = new GenericInternalRow(requiredSchema.length)
- // This gets the raw input that is parsed lately.
+ // In `PERMISSIVE` parse mode, we should be able to put the raw malformed row into the field
+ // specified in `columnNameOfCorruptRecord`. The raw input is retrieved by this method.
private def getCurrentInput(): String = tokenizer.getContext.currentParsedContent().stripLineEnd
- // This parser loads an `indexArr._1`-th position value in input tokens,
- // then put the value in `row(indexArr._2)`.
- private val indexArr: Array[(Int, Int)] = {
- val fields = if (options.dropMalformed) {
- // If `dropMalformed` is enabled, then it needs to parse all the values
- // so that we can decide which row is malformed.
- requiredSchema ++ schema.filterNot(requiredSchema.contains(_))
- } else {
- requiredSchema
- }
- // TODO: Revisit this; we need to clean up code here for readability.
- // See an URL below for related discussions:
- // https://github.com/apache/spark/pull/16928#discussion_r102636720
- val fieldsWithIndexes = fields.zipWithIndex
- corruptFieldIndex.map { case corrFieldIndex =>
- fieldsWithIndexes.filter { case (_, i) => i != corrFieldIndex }
- }.getOrElse {
- fieldsWithIndexes
- }.map { case (f, i) =>
- (dataSchema.indexOf(f), i)
- }.toArray
+ // This parser loads an `tokenIndexArr`-th position value in input tokens,
+ // then put the value in `row(rowIndexArr)`.
+ //
+ // For example, let's say there is CSV data as below:
+ //
+ // a,b,c
+ // 1,2,A
+ //
+ // Also, let's say `columnNameOfCorruptRecord` is set to "_unparsed", `header` is `true`
+ // by user and the user selects "c", "b", "_unparsed" and "a" fields. In this case, we need
+ // to map those values below:
+ //
+ // required schema - ["c", "b", "_unparsed", "a"]
+ // CSV data schema - ["a", "b", "c"]
+ // required CSV data schema - ["c", "b", "a"]
+ //
+ // with the input tokens,
+ //
+ // input tokens - [1, 2, "A"]
+ //
+ // Each input token is placed in each output row's position by mapping these. In this case,
+ //
+ // output row - ["A", 2, null, 1]
+ //
+ // In more details,
+ // - `valueConverters`, input tokens - CSV data schema
+ // `valueConverters` keeps the positions of input token indices (by its index) to each
+ // value's converter (by its value) in an order of CSV data schema. In this case,
+ // [string->int, string->int, string->string].
+ //
+ // - `tokenIndexArr`, input tokens - required CSV data schema
--- End diff --
ditto; `tokenIndexArr` is an index array in tokens corresponding to `requiredSchema`?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #17142: [SPARK-18699][SQL][FOLLOWUP] Add explanation in CSV pars...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/17142
**[Test build #73783 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/73783/testReport)** for PR 17142 at commit [`ee30cc3`](https://github.com/apache/spark/commit/ee30cc3a8445ebe62118aa9f69c3306d64b1b4a2).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org