You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "caican00 (via GitHub)" <gi...@apache.org> on 2023/07/14 02:52:24 UTC
[GitHub] [spark] caican00 opened a new pull request, #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
caican00 opened a new pull request, #41993:
URL: https://github.com/apache/spark/pull/41993
### What changes were proposed in this pull request?
The input/output Attribute is preferred for validation using the `__CHAR_VARCHAR_TYPE_STRING` type specified in its Metadata.
If not specified, use the Attribute datatype again.
### Why are the changes needed?
If the input/output Attribute specifies a `__CHAR_VARCHAR_TYPE_STRING` tag in its Metadata, then the type specified by `__CHAR_VARCHAR_TYPE_STRING` should be used for validation.
Otherwise, use the Attribute DataType for verification to avoid the following exception：
```
org.apache.spark.sql.AnalysisException: unresolved operator 'AppendData TestRelation [x#8, y#9], true;
'AppendData TestRelation [x#8, y#9], true
+- TestRelation [x#6, y#7]
```
How to reproduce？
```
val analyzer = getAnalyzer
// check varchar type
val json1 = "{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}"
val metadata1 = new MetadataBuilder().withMetadata(Metadata.fromJson(json1)).build()
val query1 = TestRelation(StructType(Seq(
StructField("x", StringType, metadata = metadata1),
StructField("y", StringType, metadata = metadata1))).toAttributes)
val table1 = TestRelation(StructType(Seq(
StructField("x", StringType, metadata = metadata1),
StructField("y", StringType, metadata = metadata1))).toAttributes)
val parsedPlanByName1 = byName(table1, query1)
analyzer.executeAndCheck(parsedPlanByName1, new QueryPlanningTracker())
```
### Does this PR introduce _any_ user-facing change?
Yes. The above code works fine.
### How was this patch tested?
new UT
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] cloud-fan commented on a diff in pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #41993:
URL: https://github.com/apache/spark/pull/41993#discussion_r1266124561
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1167,4 +1172,31 @@ class DSV2CharVarcharTestSuite extends CharVarcharTestSuite
}
}
}
+
+ test("SPARK-44414: Fixed matching check for CharType/VarcharType") {
+ Seq("{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}",
+ "{\"__CHAR_VARCHAR_TYPE_STRING\":\"char(80)\"}").foreach(json => {
+
+ // query
+ val rows = Seq(Row("1", "2"))
+ val metadata = new MetadataBuilder().withMetadata(Metadata.fromJson(json)).build()
+ val schema = StructType(Seq(
+ StructField("x", StringType, metadata = metadata),
+ StructField("y", StringType, metadata = metadata)))
Review Comment:
I'm not really sure this is an end-to-end test... `__CHAR_VARCHAR_TYPE_STRING` is an internal metadata key and we don't expect end users to set 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
[GitHub] [spark] yaooqinn commented on a diff in pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #41993:
URL: https://github.com/apache/spark/pull/41993#discussion_r1263271281
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/V2WriteAnalysisSuite.scala:
##########
@@ -744,6 +744,46 @@ abstract class V2WriteAnalysisSuiteBase extends AnalysisTest {
}
}
+ test ("SPARK-44414: Fixed matching check for CharType/VarcharType") {
+ // check varchar type
+ val json1 = "{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}"
Review Comment:
Consider using `Seq("{\"__CHAR_VARCHAR_TYPE_STRING\":\"char(80)\"}", "{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}").foreach { ... ` to minimize duplicates?
--
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-44414][SQL] Fixed matching check for CharType/VarcharType [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] closed pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
URL: https://github.com/apache/spark/pull/41993
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a diff in pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #41993:
URL: https://github.com/apache/spark/pull/41993#discussion_r1266131725
##########
sql/core/src/test/scala/org/apache/spark/sql/CharVarcharTestSuite.scala:
##########
@@ -1167,4 +1172,31 @@ class DSV2CharVarcharTestSuite extends CharVarcharTestSuite
}
}
}
+
+ test("SPARK-44414: Fixed matching check for CharType/VarcharType") {
+ Seq("{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}",
+ "{\"__CHAR_VARCHAR_TYPE_STRING\":\"char(80)\"}").foreach(json => {
+
+ // query
+ val rows = Seq(Row("1", "2"))
+ val metadata = new MetadataBuilder().withMetadata(Metadata.fromJson(json)).build()
+ val schema = StructType(Seq(
Review Comment:
You shall create `schema` directly with char/varchar, and pass it in to `createDataFrame`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on PR #41993:
URL: https://github.com/apache/spark/pull/41993#issuecomment-1635241942
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
[GitHub] [spark] cloud-fan commented on pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #41993:
URL: https://github.com/apache/spark/pull/41993#issuecomment-1637319832
Is there an end-to-end repro? char/varchar is not officially supported in the type system, or it might not work if you manually construct some arbitrary logical plans.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] caican00 commented on a diff in pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "caican00 (via GitHub)" <gi...@apache.org>.
caican00 commented on code in PR #41993:
URL: https://github.com/apache/spark/pull/41993#discussion_r1263279866
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/V2WriteAnalysisSuite.scala:
##########
@@ -744,6 +744,46 @@ abstract class V2WriteAnalysisSuiteBase extends AnalysisTest {
}
}
+ test ("SPARK-44414: Fixed matching check for CharType/VarcharType") {
+ // check varchar type
+ val json1 = "{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}"
Review Comment:
> Consider using `Seq("{\"__CHAR_VARCHAR_TYPE_STRING\":\"char(80)\"}", "{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}").foreach { ... ` to minimize duplicates?
updated. Thanks for reviewing @yaooqinn
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a diff in pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #41993:
URL: https://github.com/apache/spark/pull/41993#discussion_r1263323716
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/V2WriteAnalysisSuite.scala:
##########
@@ -744,6 +744,46 @@ abstract class V2WriteAnalysisSuiteBase extends AnalysisTest {
}
}
+ test ("SPARK-44414: Fixed matching check for CharType/VarcharType") {
+ // check varchar type
+ val json1 = "{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}"
Review Comment:
Do we also have an e2e test case to add to CharVarcharTestSuite? The reproducible test case in PR desc actually starts from the analyzer
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] [spark] yaooqinn commented on a diff in pull request #41993: [SPARK-44414][SQL] Fixed matching check for CharType/VarcharType
Posted by "yaooqinn (via GitHub)" <gi...@apache.org>.
yaooqinn commented on code in PR #41993:
URL: https://github.com/apache/spark/pull/41993#discussion_r1263323716
##########
sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/analysis/V2WriteAnalysisSuite.scala:
##########
@@ -744,6 +744,46 @@ abstract class V2WriteAnalysisSuiteBase extends AnalysisTest {
}
}
+ test ("SPARK-44414: Fixed matching check for CharType/VarcharType") {
+ // check varchar type
+ val json1 = "{\"__CHAR_VARCHAR_TYPE_STRING\":\"varchar(80)\"}"
Review Comment:
Do we also have an e2e test case to add to CharVarcharTestSuite?
--
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-44414][SQL] Fixed matching check for CharType/VarcharType [spark]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #41993:
URL: https://github.com/apache/spark/pull/41993#issuecomment-1782105166
We're closing this PR because it hasn't been updated in a while. This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and ask a committer to remove the Stale tag!
--
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