srielau commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3815834875
##########
sql/connect/client/jdbc/src/main/scala/org/apache/spark/sql/connect/client/jdbc/util/JdbcTypeUtils.scala:
##########
@@ -97,6 +99,8 @@ private[jdbc] object JdbcTypeUtils {
case LongType => 19
case FloatType => 7
case DoubleType => 15
+ case c: CharType => c.length
+ case v: VarcharType => v.length
Review Comment:
This precision mapping looks right, and `getDisplaySize` for `_: StringType`
will pick it up so Connect `getColumns` `COLUMN_SIZE` becomes `n`.
Two gaps this does not cover:
- `SparkConnectDatabaseMetaData.getColumns` still hardcodes
`CHAR_OCTET_LENGTH` to 0. Clients that size buffers from that column still see
unknown width.
- `getTypeInfo` still only lists `STRING` as `Types.VARCHAR`, so tools that
discover types from type-info never see CHAR.
In scope here, or a follow-up? Either way, please add a Connect `getColumns`
test on a CHAR/VARCHAR *table*, not only CAST result metadata.
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SparkMetadataOperationSuite.scala:
##########
@@ -338,7 +338,7 @@ class SparkMetadataOperationSuite extends
HiveThriftServer2TestBase {
val colSize = rowSet.getInt("COLUMN_SIZE")
schema(pos).dataType match {
- case StringType | BinaryType | _: ArrayType | _: MapType | _:
VarcharType =>
+ case StringType | BinaryType | _: ArrayType | _: MapType =>
assert(colSize === 0)
case o => assert(colSize === o.defaultSize)
Review Comment:
VARCHAR now falls through to `assert(colSize === o.defaultSize)`. That
equals `length` today (`VarcharType.defaultSize = length`), so `varchar(1024)`
passes for the wrong reason. `defaultSize` is the wrong contract (`STRING` is
20).
Please match `CharType` / `VarcharType` and assert `colSize === t.length`.
This test also does not enable `standardSemantics`, unlike the new
result-metadata test below.
##########
sql/hive-thriftserver/src/test/scala/org/apache/spark/sql/hive/thriftserver/SparkMetadataOperationSuite.scala:
##########
@@ -372,6 +372,23 @@ class SparkMetadataOperationSuite extends
HiveThriftServer2TestBase {
}
}
+ test("SPARK-58794: result metadata preserves CHAR and VARCHAR") {
Review Comment:
Execute-path CHAR/VARCHAR was already wired in
`SparkExecuteStatementOperation` (`TTypeId` + `CHARACTER_MAXIMUM_LENGTH` via
`defaultSize`). This test does not prove the `getColumnSize` VARCHAR change.
Fine as a thin pin of Hive JDBC names (`char` / `varchar`, no length) vs
Connect (`CHAR(4)` / `VARCHAR(6)`), but please also assert `getColumns`
`COLUMN_SIZE` for `c17` / `c18` explicitly (`255` / `1024`).
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetColumnsOperation.scala:
##########
@@ -136,6 +136,7 @@ private[hive] class SparkGetColumnsOperation(
CalendarIntervalType | NullType | _: AnsiIntervalType) =>
Some(dt.defaultSize)
case c: CharType => Some(c.length)
+ case v: VarcharType => Some(v.length)
Review Comment:
This is the actual HS2 fix (VARCHAR was already `Types.VARCHAR`; only size
was missing). The method scaladoc above still says string column size is
unknown -- please mention CHAR/VARCHAR(`n`) now return `n`.
HS2 `CHAR_OCTET_LENGTH` is still always null. Same question as Connect: fix,
or call out as out of scope?
##########
sql/connect/client/jdbc/src/test/scala/org/apache/spark/sql/connect/client/jdbc/SparkConnectJdbcDataTypeSuite.scala:
##########
@@ -223,6 +223,36 @@ class SparkConnectJdbcDataTypeSuite extends
ConnectFunSuite with RemoteSparkSess
}
}
+ test("SPARK-58794: get char and varchar types") {
Review Comment:
Good result-metadata coverage for CAST. Please also cover
`DatabaseMetaData.getColumns` on a CHAR/VARCHAR table (type code, type name,
`COLUMN_SIZE`). That path uses `getDisplaySize` for `COLUMN_SIZE` and hardcodes
`CHAR_OCTET_LENGTH` to 0, so CAST-only tests will not catch a catalog-metadata
regression.
The PR notes these suites were not run locally -- worth calling out in the
test plan / waiting on CI.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
