You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "yaooqinn (via GitHub)" <gi...@apache.org> on 2024/02/20 10:01:44 UTC
[PR] [SPARK-47101][SQL] Make HiveExternalCatalog.verifyDataSchema comply with hive column name rules [spark]
yaooqinn opened a new pull request, #45180:
URL: https://github.com/apache/spark/pull/45180
<!--
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
'common/utils/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.
-->
> In Hive 0.13 and later, column names can contain any [Unicode](http://en.wikipedia.org/wiki/List_of_Unicode_characters) character (see [HIVE-6013](https://issues.apache.org/jira/browse/HIVE-6013)), however, dot (.) and colon (:) yield errors on querying, so they are disallowed in Hive 1.2.0 (see [HIVE-10120](https://issues.apache.org/jira/browse/HIVE-10120)). Any column name that is specified within backticks (`) is treated literally. Within a backtick string, use double backticks (``) to represent a backtick character. Backtick quotation also enables the use of reserved keywords for table and column identifiers.
According to Hive Doc, the column names have the flexibility to contain any character from the Unicode set.
This PR makes HiveExternalCatalog.verifyDataSchema comply with hive column name rules.
- ',' for top-level column names is allowed
- ",:;" for nested column names are incomplete; for example, "^%", etc., are not allowed. So, we use Hive API to verify instead.
### 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.
-->
bugfix
### 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'.
-->
yes, some special characters are now allowed and errors for some invalid characters now throw Spark Errors instead of Hive Meta Errors
### 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.
-->
new tests
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
no
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497806282
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
Review Comment:
Does this replace rule came from Hive? Can we have a link?
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2877,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
+ withTable("t") {
+ checkError(
+ exception = intercept[SparkException] {
+ sql(s"CREATE TABLE t (a $typ) USING hive")
+ },
+ errorClass = "CANNOT_RECOGNIZE_HIVE_TYPE",
Review Comment:
Got it. We can reuse the existing error class in this case.
--
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
Re: [PR] [SPARK-47101][SQL] Make HiveExternalCatalog.verifyDataSchema comply with hive column name rules [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1496768986
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,22 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ Seq(",", ";", "^", "\\", "/", "%").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "")
withTable("t") {
+ val msg = s"Error: : expected at the position 16 of '$replaced' but '$c' is found."
checkError(
exception = intercept[AnalysisException] {
- spark.range(1)
- .select(struct(lit(0).as(nestedColumnName)).as("toplevel"))
- .write
- .format("hive")
- .saveAsTable("t")
+ sql(s"CREATE TABLE t (a $typ) USING hive")
},
- errorClass = "INVALID_HIVE_COLUMN_NAME",
+ errorClass = "INVALID_HIVE_COLUMN_TYPE",
parameters = Map(
- "invalidChars" -> "',', ':', ';'",
+ "detailMessage" -> msg,
Review Comment:
Thank you for the information
--
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
Re: [PR] [SPARK-47101][SQL] Make HiveExternalCatalog.verifyDataSchema comply with hive column name rules [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1955736210
> Does this comply with other RDBMSes too? I'm curious if this is another Hive esoteric feature or not.
This does not change the parser layer, which means we already have the capability to handle special characters in the column names. This schema verification happens only in the hive catalog, while v1 in-memory, v2 jdbc, and other catalogs are free to use any character in column names.
We are not intended to comply with user behavior on Hive, but rather with underlying restrictions when calling HMS APIs.
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497399190
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3385,24 +3402,16 @@ class HiveDDLSuite
}
}
- test("SPARK-44911: Create the table with invalid column") {
+ test("SPARK-47101: comma is allowed in column name") {
val tbl = "t1"
withTable(tbl) {
- val e = intercept[AnalysisException] {
- sql(
- s"""
- |CREATE TABLE t1
- |STORED AS parquet
- |SELECT id, DATE'2018-01-01' + MAKE_DT_INTERVAL(0, id) FROM RANGE(0, 10)
+ sql(
+ s"""
+ |CREATE TABLE t1
+ |STORED AS parquet
+ |SELECT id, DATE'2018-11-17' + MAKE_DT_INTERVAL(0, id) FROM RANGE(13, 14)
Review Comment:
can we make the column name more explicit? the auto-generated alias may change over time.
--
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
Re: [PR] [SPARK-47101][SQL] Make `HiveExternalCatalog.verifyDataSchema` comply with hive column name rules [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1955811280
Also, cc @cloud-fan
--
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
Re: [PR] [SPARK-47101][SQL] Make `HiveExternalCatalog.verifyDataSchema` comply with hive column name rules [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1956010268
Ur, for the above example, it looks like unsafe in URL (S3 or Web URL based Hadoop-compatible file system). Can we use `,` in the middle of URI (except the file name part)?
```
spark-warehouse/a
└── a,b=2
├── part-00000-b75cb28d-3fb0-4858-b93d-3f089d3e63b4.c000
└── part-00000-e558ae00-dcae-4025-bc6f-819a1debf209.c000
```
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and use `TypeInfoUtils.getTypeInfoFromTypeString` to check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1956083959
Hi @dongjoon-hyun, I updated the title and PR description; please check if they are clearer or too earful.
--
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
Re: [PR] [SPARK-47101][SQL] Make `HiveExternalCatalog.verifyDataSchema` comply with hive column name rules [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1955991831
Hi @dongjoon-hyun.
- This PR allows the use of commas in column names.
- In contrast, we don't disallow any more special characters to be used in nested type information. Because it will eventually fail for HMS calls `TypeInfoUtils.getTypeInfoFromTypeString`, we just bring this step forward where we did for ",:;" before.
It might be necessary to verify that commas can be safely used in partition names, as they are allowed in column names.
```
create table a(`a,b` int, c int) using hive PARTITIONED BY (`a,b`);
insert into a values(1, 2);
select * from a;
#output
1 2
```
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1958543240
Merged to master for Apache Spark 4.0.0.
--
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
Re: [PR] [SPARK-47101][SQL] Make `HiveExternalCatalog.verifyDataSchema` comply with hive column name rules [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1956075113
> Ur, for the above example, it looks like unsafe in URL (S3 or Web URL based Hadoop-compatible file system). Can we use , in the middle of URI (except the file name part)?
https://docs.aws.amazon.com/AmazonS3/latest/userguide/object-keys.html#object-key-guidelines
I see Comma(",") and Equals ("=") in the same group of `Characters that might require special handling`. As `=` is always there for partition keys, so it's safe?
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497812468
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
+ withTable("t") {
+ checkError(
+ exception = intercept[SparkException] {
+ sql(s"CREATE TABLE t (a $typ) USING hive")
+ },
+ errorClass = "CANNOT_RECOGNIZE_HIVE_TYPE",
+ parameters = Map(
+ "fieldType" -> toSQLType(replaced),
+ "fieldName" -> "`a`")
+ )
+ }
+ }
+ // other special characters
+ Seq(";", "^", "\\", "/", "%").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "")
+ val msg = s"java.lang.IllegalArgumentException: Error: : expected at the position " +
+ s"16 of '$replaced' but '$c' is found."
withTable("t") {
checkError(
exception = intercept[AnalysisException] {
- spark.range(1)
- .select(struct(lit(0).as(nestedColumnName)).as("toplevel"))
- .write
- .format("hive")
- .saveAsTable("t")
+ sql(s"CREATE TABLE t (a $typ) USING hive")
},
- errorClass = "INVALID_HIVE_COLUMN_NAME",
+ errorClass = "_LEGACY_ERROR_TEMP_3065",
Review Comment:
Why does this PR switch from `INVALID_HIVE_COLUMN_NAME` to `_LEGACY_ERROR_TEMP_3065`?
Can we exclude the deletion of `INVALID_HIVE_COLUMN_NAME` from this PR?
- docs/sql-error-conditions.md
- common/utils/src/main/resources/error/error-classes.json
--
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
Re: [PR] [SPARK-47101][SQL] Make `HiveExternalCatalog.verifyDataSchema` comply with hive column name rules [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1956007752
Thank you. Could you revise the PR title to specifically narrow down to the following additional contribution instead of saying `hive column name rules`?
> This PR allows the use of commas in column 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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1498521461
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
Review Comment:
OK
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1958548862
Thank you @dongjoon-hyun @cloud-fan @MaxGekk
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1502626283
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
Review Comment:
I feel it's clearer to write the string literal of the replaced value, instead of using this complex regex.
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1503547763
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,39 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ // The regex is from HiveClientImpl.getSparkSQLDataType, please keep them in sync.
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
+ withTable("t") {
+ checkError(
+ exception = intercept[SparkException] {
+ sql(s"CREATE TABLE t (a $typ) USING hive")
Review Comment:
Still fine
--
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
Re: [PR] [SPARK-47101][SQL] Make HiveExternalCatalog.verifyDataSchema comply with hive column name rules [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1496168699
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,22 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ Seq(",", ";", "^", "\\", "/", "%").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "")
withTable("t") {
+ val msg = s"Error: : expected at the position 16 of '$replaced' but '$c' is found."
checkError(
exception = intercept[AnalysisException] {
- spark.range(1)
- .select(struct(lit(0).as(nestedColumnName)).as("toplevel"))
- .write
- .format("hive")
- .saveAsTable("t")
+ sql(s"CREATE TABLE t (a $typ) USING hive")
},
- errorClass = "INVALID_HIVE_COLUMN_NAME",
+ errorClass = "INVALID_HIVE_COLUMN_TYPE",
parameters = Map(
- "invalidChars" -> "',', ':', ';'",
+ "detailMessage" -> msg,
Review Comment:
We should avoid embedding arbitrary text as parameters.
- If you want provide more details, just put the cause exception as `cause` to `AnalysisException`.
- clients might reassemble error messages from parameters and show it in different languages.
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and use `TypeInfoUtils.getTypeInfoFromTypeString` to check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497068367
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -151,45 +152,35 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
/**
- * Checks the validity of data column names. Hive metastore disallows the table to use some
- * special characters (',', ':', and ';') in data column names, including nested column names.
- * Partition columns do not have such a restriction. Views do not have such a restriction.
+ * According to the Hive Document:
+ * > In Hive 0.13 and later, column names can contain any Unicode character (see HIVE-6013),
+ * however, dot (.) and colon (:) yield errors on querying, so they are disallowed in
+ * Hive 1.2.0 (see HIVE-10120). Any column name that is specified within backticks (`) is
+ * treated literally. Within a backtick string, use double backticks (``) to represent
+ * a backtick character. Backtick quotation also enables the use of reserved keywords
+ * for table and column identifiers
+ * In addition, Spark SQL doesn't rely on Hive analysis for column resolution, so dot (.) and
+ * colon (:) yield no errors on querying.
+ *
+ * To sum up, we do not need to check top level column names, and check the nested types that
+ * contain a 'name' field in it.
+ *
*/
private def verifyDataSchema(
tableName: TableIdentifier, tableType: CatalogTableType, dataSchema: StructType): Unit = {
if (tableType != VIEW) {
- val invalidChars = Seq(",", ":", ";")
- def verifyNestedColumnNames(schema: StructType): Unit = schema.foreach { f =>
- f.dataType match {
- case st: StructType => verifyNestedColumnNames(st)
- case _ if invalidChars.exists(f.name.contains) =>
- val invalidCharsString = invalidChars.map(c => s"'$c'").mkString(", ")
- throw new AnalysisException(
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- messageParameters = Map(
- "invalidChars" -> invalidCharsString,
- "tableName" -> toSQLId(tableName.nameParts),
- "columnName" -> toSQLId(f.name)
- ))
- case _ =>
- }
- }
-
dataSchema.foreach { f =>
- f.dataType match {
- // Checks top-level column names
- case _ if f.name.contains(",") =>
+ try {
+ TypeInfoUtils.getTypeInfoFromTypeString(f.dataType.catalogString)
Review Comment:
what does it do? I can't find it in the previous code.
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and use `TypeInfoUtils.getTypeInfoFromTypeString` to check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497068367
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -151,45 +152,35 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
/**
- * Checks the validity of data column names. Hive metastore disallows the table to use some
- * special characters (',', ':', and ';') in data column names, including nested column names.
- * Partition columns do not have such a restriction. Views do not have such a restriction.
+ * According to the Hive Document:
+ * > In Hive 0.13 and later, column names can contain any Unicode character (see HIVE-6013),
+ * however, dot (.) and colon (:) yield errors on querying, so they are disallowed in
+ * Hive 1.2.0 (see HIVE-10120). Any column name that is specified within backticks (`) is
+ * treated literally. Within a backtick string, use double backticks (``) to represent
+ * a backtick character. Backtick quotation also enables the use of reserved keywords
+ * for table and column identifiers
+ * In addition, Spark SQL doesn't rely on Hive analysis for column resolution, so dot (.) and
+ * colon (:) yield no errors on querying.
+ *
+ * To sum up, we do not need to check top level column names, and check the nested types that
+ * contain a 'name' field in it.
+ *
*/
private def verifyDataSchema(
tableName: TableIdentifier, tableType: CatalogTableType, dataSchema: StructType): Unit = {
if (tableType != VIEW) {
- val invalidChars = Seq(",", ":", ";")
- def verifyNestedColumnNames(schema: StructType): Unit = schema.foreach { f =>
- f.dataType match {
- case st: StructType => verifyNestedColumnNames(st)
- case _ if invalidChars.exists(f.name.contains) =>
- val invalidCharsString = invalidChars.map(c => s"'$c'").mkString(", ")
- throw new AnalysisException(
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- messageParameters = Map(
- "invalidChars" -> invalidCharsString,
- "tableName" -> toSQLId(tableName.nameParts),
- "columnName" -> toSQLId(f.name)
- ))
- case _ =>
- }
- }
-
dataSchema.foreach { f =>
- f.dataType match {
- // Checks top-level column names
- case _ if f.name.contains(",") =>
+ try {
+ TypeInfoUtils.getTypeInfoFromTypeString(f.dataType.catalogString)
Review Comment:
what does it do? I can't find it in the previous code?
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497815361
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3385,24 +3401,11 @@ class HiveDDLSuite
}
}
- test("SPARK-44911: Create the table with invalid column") {
+ test("SPARK-47101: comma is allowed in column name") {
val tbl = "t1"
withTable(tbl) {
- val e = intercept[AnalysisException] {
- sql(
- s"""
- |CREATE TABLE t1
- |STORED AS parquet
- |SELECT id, DATE'2018-01-01' + MAKE_DT_INTERVAL(0, id) FROM RANGE(0, 10)
- """.stripMargin)
- }
- checkError(e,
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- parameters = Map(
- "invalidChars" -> "','",
- "tableName" -> "`spark_catalog`.`default`.`t1`",
- "columnName" -> "`DATE '2018-01-01' + make_dt_interval(0, id, 0, 0`.`000000)`")
- )
+ sql("CREATE TABLE t1 STORED AS parquet SELECT id as `a,b` FROM range(1)")
Review Comment:
Thank you for adding this simpler version.
However, if you don't mind, shall we keep the existing test case, too?
```
SELECT id, DATE'2018-01-01' + MAKE_DT_INTERVAL(0, id) FROM RANGE(0, 10)
```
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497467424
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2877,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
+ withTable("t") {
+ checkError(
+ exception = intercept[SparkException] {
+ sql(s"CREATE TABLE t (a $typ) USING hive")
+ },
+ errorClass = "CANNOT_RECOGNIZE_HIVE_TYPE",
Review Comment:
In `HiveClientImpl.getSparkSQLDataType`
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1502624159
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,39 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
Review Comment:
I'm a bit confused, why don't we just write the string literal of the replaced value? It's hard to figure out the final type string we are testing here.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,39 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
Review Comment:
I'm a bit confused, why don't we just write the string literal of the replaced value? It's hard to figure out the final type string we are testing 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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1502627813
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,39 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ // The regex is from HiveClientImpl.getSparkSQLDataType, please keep them in sync.
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
+ withTable("t") {
+ checkError(
+ exception = intercept[SparkException] {
+ sql(s"CREATE TABLE t (a $typ) USING hive")
Review Comment:
for parquet tables, do we still have this error?
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1498524228
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2876,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
+ withTable("t") {
+ checkError(
+ exception = intercept[SparkException] {
+ sql(s"CREATE TABLE t (a $typ) USING hive")
+ },
+ errorClass = "CANNOT_RECOGNIZE_HIVE_TYPE",
+ parameters = Map(
+ "fieldType" -> toSQLType(replaced),
+ "fieldName" -> "`a`")
+ )
+ }
+ }
+ // other special characters
+ Seq(";", "^", "\\", "/", "%").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "")
+ val msg = s"java.lang.IllegalArgumentException: Error: : expected at the position " +
+ s"16 of '$replaced' but '$c' is found."
withTable("t") {
checkError(
exception = intercept[AnalysisException] {
- spark.range(1)
- .select(struct(lit(0).as(nestedColumnName)).as("toplevel"))
- .write
- .format("hive")
- .saveAsTable("t")
+ sql(s"CREATE TABLE t (a $typ) USING hive")
},
- errorClass = "INVALID_HIVE_COLUMN_NAME",
+ errorClass = "_LEGACY_ERROR_TEMP_3065",
Review Comment:
INVALID_HIVE_COLUMN_NAME is not necessary anymore. 1) the restrictions for column names have been removed in this PR. 2) Nested field names belong to the data type part. For these two reasons, INVALID_HIVE_COLUMN_NAME could be removed.
_LEGACY_ERROR_TEMP_3065 is thrown by `org.apache.spark.sql.hive.HiveExternalCatalog#withClient`. It's hard to distinguish one Hive error from another for metastore API calls.
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3385,24 +3401,11 @@ class HiveDDLSuite
}
}
- test("SPARK-44911: Create the table with invalid column") {
+ test("SPARK-47101: comma is allowed in column name") {
val tbl = "t1"
withTable(tbl) {
- val e = intercept[AnalysisException] {
- sql(
- s"""
- |CREATE TABLE t1
- |STORED AS parquet
- |SELECT id, DATE'2018-01-01' + MAKE_DT_INTERVAL(0, id) FROM RANGE(0, 10)
- """.stripMargin)
- }
- checkError(e,
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- parameters = Map(
- "invalidChars" -> "','",
- "tableName" -> "`spark_catalog`.`default`.`t1`",
- "columnName" -> "`DATE '2018-01-01' + make_dt_interval(0, id, 0, 0`.`000000)`")
- )
+ sql("CREATE TABLE t1 STORED AS parquet SELECT id as `a,b` FROM range(1)")
Review Comment:
Hi @dongjoon-hyun, this is changed via request from @cloud-fan https://github.com/apache/spark/pull/45180#discussion_r1497399190
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1498535180
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -3385,24 +3401,11 @@ class HiveDDLSuite
}
}
- test("SPARK-44911: Create the table with invalid column") {
+ test("SPARK-47101: comma is allowed in column name") {
val tbl = "t1"
withTable(tbl) {
- val e = intercept[AnalysisException] {
- sql(
- s"""
- |CREATE TABLE t1
- |STORED AS parquet
- |SELECT id, DATE'2018-01-01' + MAKE_DT_INTERVAL(0, id) FROM RANGE(0, 10)
- """.stripMargin)
- }
- checkError(e,
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- parameters = Map(
- "invalidChars" -> "','",
- "tableName" -> "`spark_catalog`.`default`.`t1`",
- "columnName" -> "`DATE '2018-01-01' + make_dt_interval(0, id, 0, 0`.`000000)`")
- )
+ sql("CREATE TABLE t1 STORED AS parquet SELECT id as `a,b` FROM range(1)")
Review Comment:
Ah, got it~
--
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
Re: [PR] [SPARK-47101][SQL] Make HiveExternalCatalog.verifyDataSchema comply with hive column name rules [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on PR #45180:
URL: https://github.com/apache/spark/pull/45180#issuecomment-1955745217
Got it. Thank you for the clarification~
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and use `TypeInfoUtils.getTypeInfoFromTypeString` to check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497076486
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -151,45 +152,35 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
/**
- * Checks the validity of data column names. Hive metastore disallows the table to use some
- * special characters (',', ':', and ';') in data column names, including nested column names.
- * Partition columns do not have such a restriction. Views do not have such a restriction.
+ * According to the Hive Document:
+ * > In Hive 0.13 and later, column names can contain any Unicode character (see HIVE-6013),
+ * however, dot (.) and colon (:) yield errors on querying, so they are disallowed in
+ * Hive 1.2.0 (see HIVE-10120). Any column name that is specified within backticks (`) is
+ * treated literally. Within a backtick string, use double backticks (``) to represent
+ * a backtick character. Backtick quotation also enables the use of reserved keywords
+ * for table and column identifiers
+ * In addition, Spark SQL doesn't rely on Hive analysis for column resolution, so dot (.) and
+ * colon (:) yield no errors on querying.
+ *
+ * To sum up, we do not need to check top level column names, and check the nested types that
+ * contain a 'name' field in it.
+ *
*/
private def verifyDataSchema(
tableName: TableIdentifier, tableType: CatalogTableType, dataSchema: StructType): Unit = {
if (tableType != VIEW) {
- val invalidChars = Seq(",", ":", ";")
- def verifyNestedColumnNames(schema: StructType): Unit = schema.foreach { f =>
- f.dataType match {
- case st: StructType => verifyNestedColumnNames(st)
- case _ if invalidChars.exists(f.name.contains) =>
- val invalidCharsString = invalidChars.map(c => s"'$c'").mkString(", ")
- throw new AnalysisException(
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- messageParameters = Map(
- "invalidChars" -> invalidCharsString,
- "tableName" -> toSQLId(tableName.nameParts),
- "columnName" -> toSQLId(f.name)
- ))
- case _ =>
- }
- }
-
dataSchema.foreach { f =>
- f.dataType match {
- // Checks top-level column names
- case _ if f.name.contains(",") =>
+ try {
+ TypeInfoUtils.getTypeInfoFromTypeString(f.dataType.catalogString)
Review Comment:
This tokenizes the input, such as `string`, `struct<ab:int>`, and then parses it to `org.apache.hadoop.hive.serde2.typeinfo.TypeInfo`
--
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
Re: [PR] [SPARK-47101][SQL] Make HiveExternalCatalog.verifyDataSchema comply with hive column name rules [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1496127149
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -151,45 +152,35 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
/**
- * Checks the validity of data column names. Hive metastore disallows the table to use some
- * special characters (',', ':', and ';') in data column names, including nested column names.
- * Partition columns do not have such a restriction. Views do not have such a restriction.
+ * According to the Hive Document:
+ * > In Hive 0.13 and later, column names can contain any Unicode character (see HIVE-6013),
+ * however, dot (.) and colon (:) yield errors on querying, so they are disallowed in
+ * Hive 1.2.0 (see HIVE-10120). Any column name that is specified within backticks (`) is
+ * treated literally. Within a backtick string, use double backticks (``) to represent
+ * a backtick character. Backtick quotation also enables the use of reserved keywords
+ * for table and column identifiers
+ * In addition, Spark SQL doesn't rely on Hive analysis for column resolution, so dot (.) and
+ * colon (:) yield no errors on querying.
+ *
+ * To sum up, we do not need to check top level column names, and check the nested types that
+ * contain a 'name' field in it.
+ *
*/
private def verifyDataSchema(
Review Comment:
In addition, this module is under `src/main`. This cannot be a part of `TEST` module. Please fix the JIRA issue type too.
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and use `TypeInfoUtils.getTypeInfoFromTypeString` to check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497090580
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -151,45 +152,35 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
/**
- * Checks the validity of data column names. Hive metastore disallows the table to use some
- * special characters (',', ':', and ';') in data column names, including nested column names.
- * Partition columns do not have such a restriction. Views do not have such a restriction.
+ * According to the Hive Document:
+ * > In Hive 0.13 and later, column names can contain any Unicode character (see HIVE-6013),
+ * however, dot (.) and colon (:) yield errors on querying, so they are disallowed in
+ * Hive 1.2.0 (see HIVE-10120). Any column name that is specified within backticks (`) is
+ * treated literally. Within a backtick string, use double backticks (``) to represent
+ * a backtick character. Backtick quotation also enables the use of reserved keywords
+ * for table and column identifiers
+ * In addition, Spark SQL doesn't rely on Hive analysis for column resolution, so dot (.) and
+ * colon (:) yield no errors on querying.
+ *
+ * To sum up, we do not need to check top level column names, and check the nested types that
+ * contain a 'name' field in it.
+ *
*/
private def verifyDataSchema(
tableName: TableIdentifier, tableType: CatalogTableType, dataSchema: StructType): Unit = {
if (tableType != VIEW) {
- val invalidChars = Seq(",", ":", ";")
- def verifyNestedColumnNames(schema: StructType): Unit = schema.foreach { f =>
- f.dataType match {
- case st: StructType => verifyNestedColumnNames(st)
- case _ if invalidChars.exists(f.name.contains) =>
- val invalidCharsString = invalidChars.map(c => s"'$c'").mkString(", ")
- throw new AnalysisException(
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- messageParameters = Map(
- "invalidChars" -> invalidCharsString,
- "tableName" -> toSQLId(tableName.nameParts),
- "columnName" -> toSQLId(f.name)
- ))
- case _ =>
- }
- }
-
dataSchema.foreach { f =>
- f.dataType match {
- // Checks top-level column names
- case _ if f.name.contains(",") =>
+ try {
+ TypeInfoUtils.getTypeInfoFromTypeString(f.dataType.catalogString)
Review Comment:
why do we need this new check now?
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "dongjoon-hyun (via GitHub)" <gi...@apache.org>.
dongjoon-hyun closed pull request #45180: [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema`
URL: https://github.com/apache/spark/pull/45180
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and use `TypeInfoUtils.getTypeInfoFromTypeString` to check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497126709
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -151,45 +152,35 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
/**
- * Checks the validity of data column names. Hive metastore disallows the table to use some
- * special characters (',', ':', and ';') in data column names, including nested column names.
- * Partition columns do not have such a restriction. Views do not have such a restriction.
+ * According to the Hive Document:
+ * > In Hive 0.13 and later, column names can contain any Unicode character (see HIVE-6013),
+ * however, dot (.) and colon (:) yield errors on querying, so they are disallowed in
+ * Hive 1.2.0 (see HIVE-10120). Any column name that is specified within backticks (`) is
+ * treated literally. Within a backtick string, use double backticks (``) to represent
+ * a backtick character. Backtick quotation also enables the use of reserved keywords
+ * for table and column identifiers
+ * In addition, Spark SQL doesn't rely on Hive analysis for column resolution, so dot (.) and
+ * colon (:) yield no errors on querying.
+ *
+ * To sum up, we do not need to check top level column names, and check the nested types that
+ * contain a 'name' field in it.
+ *
*/
private def verifyDataSchema(
tableName: TableIdentifier, tableType: CatalogTableType, dataSchema: StructType): Unit = {
if (tableType != VIEW) {
- val invalidChars = Seq(",", ":", ";")
- def verifyNestedColumnNames(schema: StructType): Unit = schema.foreach { f =>
- f.dataType match {
- case st: StructType => verifyNestedColumnNames(st)
- case _ if invalidChars.exists(f.name.contains) =>
- val invalidCharsString = invalidChars.map(c => s"'$c'").mkString(", ")
- throw new AnalysisException(
- errorClass = "INVALID_HIVE_COLUMN_NAME",
- messageParameters = Map(
- "invalidChars" -> invalidCharsString,
- "tableName" -> toSQLId(tableName.nameParts),
- "columnName" -> toSQLId(f.name)
- ))
- case _ =>
- }
- }
-
dataSchema.foreach { f =>
- f.dataType match {
- // Checks top-level column names
- case _ if f.name.contains(",") =>
+ try {
+ TypeInfoUtils.getTypeInfoFromTypeString(f.dataType.catalogString)
Review Comment:
Ah, you're absolutely right. We don't need this check, then it seems that we can remove verifyDataSchema entirely
--
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
Re: [PR] [SPARK-47101][SQL] Allow comma to be used in top-level column names and remove check nested type definition in `HiveExternalCatalog.verifyDataSchema` [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1497399800
##########
sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveDDLSuite.scala:
##########
@@ -2876,22 +2877,38 @@ class HiveDDLSuite
}
}
- test("SPARK-24681 checks if nested column names do not include ',', ':', and ';'") {
- Seq("nested,column", "nested:column", "nested;column").foreach { nestedColumnName =>
+ test("SPARK-47101 checks if nested column names do not include invalid characters") {
+ // delimiter characters
+ Seq(",", ":").foreach { c =>
+ val typ = s"array<struct<`abc${c}xyz`:int>>"
+ val replaced = typ.replaceAll("`", "").replaceAll("(?<=struct<|,)([^,<:]+)(?=:)", "`$1`")
+ withTable("t") {
+ checkError(
+ exception = intercept[SparkException] {
+ sql(s"CREATE TABLE t (a $typ) USING hive")
+ },
+ errorClass = "CANNOT_RECOGNIZE_HIVE_TYPE",
Review Comment:
just for my education, where do we throw this error?
--
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
Re: [PR] [SPARK-47101][SQL] Make HiveExternalCatalog.verifyDataSchema comply with hive column name rules [spark]
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #45180:
URL: https://github.com/apache/spark/pull/45180#discussion_r1496768832
##########
sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala:
##########
@@ -151,45 +152,35 @@ private[spark] class HiveExternalCatalog(conf: SparkConf, hadoopConf: Configurat
}
/**
- * Checks the validity of data column names. Hive metastore disallows the table to use some
- * special characters (',', ':', and ';') in data column names, including nested column names.
- * Partition columns do not have such a restriction. Views do not have such a restriction.
+ * According to the Hive Document:
+ * > In Hive 0.13 and later, column names can contain any Unicode character (see HIVE-6013),
+ * however, dot (.) and colon (:) yield errors on querying, so they are disallowed in
+ * Hive 1.2.0 (see HIVE-10120). Any column name that is specified within backticks (`) is
+ * treated literally. Within a backtick string, use double backticks (``) to represent
+ * a backtick character. Backtick quotation also enables the use of reserved keywords
+ * for table and column identifiers
+ * In addition, Spark SQL doesn't rely on Hive analysis for column resolution, so dot (.) and
+ * colon (:) yield no errors on querying.
+ *
+ * To sum up, we do not need to check top level column names, and check the nested types that
+ * contain a 'name' field in it.
+ *
*/
private def verifyDataSchema(
Review Comment:
Thank you @dongjoon-hyun. I seem to remember changing it;)
--
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