You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by viirya <gi...@git.apache.org> on 2015/03/30 13:08:08 UTC
[GitHub] spark pull request: [SPARK-6607][SQL] Aggregation attribute name i...
GitHub user viirya opened a pull request:
https://github.com/apache/spark/pull/5263
[SPARK-6607][SQL] Aggregation attribute name including special chars '(' and ')' should be replaced before generating Parquet schema
'(' and ')' are special characters used in Parquet schema for type annotation. When we run an aggregation query, we will obtain attribute name such as "MAX(a)".
If we directly store the generated DataFrame as Parquet file, it causes failure when reading and parsing the stored schema string.
Several methods can be adopted to solve this. This pr uses a simplest one to just replace attribute names before generating Parquet schema based on these attributes.
Another possible method might be modifying all aggregation expression names from "func(column)" to "func[column]".
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/viirya/spark-1 parquet_aggregation_name
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/5263.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 #5263
----
commit 1de001d375d06ec681a2ac4eb3a62f01310af21d
Author: Liang-Chi Hsieh <vi...@gmail.com>
Date: 2015-03-30T11:05:26Z
Replace special characters '(' and ')' of Parquet schema.
----
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-89504716
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29702/
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: [SPARK-6607][SQL] Check invalid characters for...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-90082734
@liancheng ok, I updated the description.
---
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: [SPARK-6607][SQL] Check invalid characters for...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-90062984
Hey @viirya, sorry that I forgot to ask you to update the PR description. Although it has already been in the Git history, would you mind to update the description for future reference on GitHub?
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-87722138
@liancheng your suggestion is okay for me. If others have no opinion for that, I will send updates later for your suggestion.
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-88059416
[Test build #29475 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29475/consoleFull) for PR 5263 at commit [`463dff4`](https://github.com/apache/spark/commit/463dff4ab0c0321e12310be00fa05788beb5a3d8).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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: [SPARK-6607][SQL] Check invalid characters for...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-90147723
@viirya Thanks!
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-89125866
@liancheng is it ready to go now?
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-89375604
Now LGTM except for two minor styling issue.
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-87661942
[Test build #29395 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29395/consoleFull) for PR 5263 at commit [`1de001d`](https://github.com/apache/spark/commit/1de001d375d06ec681a2ac4eb3a62f01310af21d).
* This patch **passes all tests**.
* This patch merges cleanly.
* This patch adds no public classes.
* This patch does not change any dependencies.
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/5263#discussion_r27744000
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/parquet/ParquetTypes.scala ---
@@ -404,7 +405,16 @@ private[parquet] object ParquetTypesConverter extends Logging {
}
}
+ def checkSpecialCharacters(schema: Seq[Attribute]) = {
+ // ,;{}()\n\t= and space character are special characters in Parquet schema
+ if (schema.exists(_.name.matches(".*[ ,;{}()\n\t=].*"))) {
+ sys.error("""Attribute name can not contain any character among " ,;{}()\n\t=".
+ | Use Alias to rename attribute name""".stripMargin)
--- End diff --
Let's include the attribute name in the error message to make it more clear, and reformat the code a bit:
```scala
private def checkSpecialCharacter(schema: Seq[Attribute]) = {
schema.map(_.name).foreach { name =>
if (name.matches(".*[ ,;{}()\n\t=].*")) {
sys.error(
s"""Attribute name "$name" contains invalid character(s) among " ,;{}()\n\t=".
|Please use alias to rename it.
""".stripMargin.split("\n").mkString)
}
}
}
```
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-87637278
[Test build #29395 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29395/consoleFull) for PR 5263 at commit [`1de001d`](https://github.com/apache/spark/commit/1de001d375d06ec681a2ac4eb3a62f01310af21d).
---
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: [SPARK-6607][SQL] Check invalid characters for...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-89608688
Thanks for working on this! 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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-87661966
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29395/
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-87696048
This is a good point. Actually all these characters ` ,;{}()\n\t=` (note there is a space character at the beginning) can be problematic if they appear in field names, according to [`MessageTypeParser`] [1].
However, personally I think simply replacing these characters with legitimate ones like brackets might be confusing. On the other hand, similar problems can be worked around easily by assigning an alias. So how about this:
1. Check all field names for invalid characters in `convertFromAttributes`
2. Throw an error message when any invalid character is found
3. In the error message, suggest the user to add an alias to the field explicitly
[1]: https://github.com/apache/incubator-parquet-mr/blob/b8f5d89e0f4347ce54cf680bd7dffc9bc02f876a/parquet-column/src/main/java/parquet/schema/MessageTypeParser.java#L46
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-88059436
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/29475/
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on a diff in the pull request:
https://github.com/apache/spark/pull/5263#discussion_r27744002
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/parquet/ParquetTypes.scala ---
@@ -404,7 +405,16 @@ private[parquet] object ParquetTypesConverter extends Logging {
}
}
+ def checkSpecialCharacters(schema: Seq[Attribute]) = {
--- End diff --
Let's make this private.
---
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: [SPARK-6607][SQL] Check invalid characters for...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/5263
---
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: [SPARK-6607][SQL] Aggregation attribute name i...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/5263#issuecomment-88032847
[Test build #29475 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/29475/consoleFull) for PR 5263 at commit [`463dff4`](https://github.com/apache/spark/commit/463dff4ab0c0321e12310be00fa05788beb5a3d8).
---
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