Skip to content

[SPARK-42904][SQL] Char/Varchar Support for JDBC Catalog#40531

Closed
yaooqinn wants to merge 1 commit into
apache:masterfrom
yaooqinn:SPARK-42904
Closed

[SPARK-42904][SQL] Char/Varchar Support for JDBC Catalog#40531
yaooqinn wants to merge 1 commit into
apache:masterfrom
yaooqinn:SPARK-42904

Conversation

@yaooqinn
Copy link
Copy Markdown
Member

@yaooqinn yaooqinn commented Mar 23, 2023

What changes were proposed in this pull request?

Add type mapping for spark char/varchar to jdbc types.

Why are the changes needed?

The STANDARD JDBC 1.0 and other modern databases define char/varchar normatively.

This is currently a kind of bug for DDLs on JDBCCatalogs for encountering errors like

Cause: org.apache.spark.SparkIllegalArgumentException: Can't get JDBC type for varchar(10).
[info]   at org.apache.spark.sql.errors.QueryExecutionErrors$.cannotGetJdbcTypeError(QueryExecutionErrors.scala:1005)

Does this PR introduce any user-facing change?

yes, char/varchar are allow for jdbc catalogs

How was this patch tested?

new ut

@github-actions github-actions Bot added the SQL label Mar 23, 2023
@yaooqinn
Copy link
Copy Markdown
Member Author

cc @HyukjinKwon @cloud-fan @dongjoon-hyun thanks

sql("CREATE TABLE h2.test.new_table(c CHAR(1000000001))")
}
assert(e.getCause.getMessage.contains("1000000001"))
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also add a VARCHAR case? like

sql("CREATE TABLE h2.test.new_table(c VARCHAR(1000000001))")

?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really necessary, as we don't classify the exception on our side

@yaooqinn yaooqinn closed this in 18cd801 Mar 24, 2023
yaooqinn added a commit that referenced this pull request Mar 24, 2023
### What changes were proposed in this pull request?

Add type mapping for spark char/varchar to jdbc types.

### Why are the changes needed?

The STANDARD JDBC 1.0 and other modern databases define char/varchar normatively.

This is currently a kind of bug for DDLs on JDBCCatalogs for encountering errors like

```
Cause: org.apache.spark.SparkIllegalArgumentException: Can't get JDBC type for varchar(10).
[info]   at org.apache.spark.sql.errors.QueryExecutionErrors$.cannotGetJdbcTypeError(QueryExecutionErrors.scala:1005)

```

### Does this PR introduce _any_ user-facing change?

yes, char/varchar are allow for jdbc catalogs

### How was this patch tested?

new ut

Closes #40531 from yaooqinn/SPARK-42904.

Authored-by: Kent Yao <yao@apache.org>
Signed-off-by: Kent Yao <yao@apache.org>
(cherry picked from commit 18cd801)
Signed-off-by: Kent Yao <yao@apache.org>
@yaooqinn
Copy link
Copy Markdown
Member Author

thanks, merged to master and 3.4

@yaooqinn yaooqinn deleted the SPARK-42904 branch March 24, 2023 06:43
snmvaughan pushed a commit to snmvaughan/spark that referenced this pull request Jun 20, 2023
### What changes were proposed in this pull request?

Add type mapping for spark char/varchar to jdbc types.

### Why are the changes needed?

The STANDARD JDBC 1.0 and other modern databases define char/varchar normatively.

This is currently a kind of bug for DDLs on JDBCCatalogs for encountering errors like

```
Cause: org.apache.spark.SparkIllegalArgumentException: Can't get JDBC type for varchar(10).
[info]   at org.apache.spark.sql.errors.QueryExecutionErrors$.cannotGetJdbcTypeError(QueryExecutionErrors.scala:1005)

```

### Does this PR introduce _any_ user-facing change?

yes, char/varchar are allow for jdbc catalogs

### How was this patch tested?

new ut

Closes apache#40531 from yaooqinn/SPARK-42904.

Authored-by: Kent Yao <yao@apache.org>
Signed-off-by: Kent Yao <yao@apache.org>
(cherry picked from commit 18cd801)
Signed-off-by: Kent Yao <yao@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants