You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "srielau (via GitHub)" <gi...@apache.org> on 2026/08/19 17:35:56 UTC
[PR] [SPARK-58794][SQL] Expose CHAR/VARCHAR metadata to SQL clients [spark]
srielau opened a new pull request, #58132:
URL: https://github.com/apache/spark/pull/58132
### What changes were proposed in this pull request?
**Depends on #58130**, which depends on #58087 / #58080 / #58033. Please review and merge those first; rebase this onto `master` afterward. Delta vs follow-up: https://github.com/srielau/spark/compare/serge-rielau_data/SPARK-58794-r1-implicit...serge-rielau_data/SPARK-58794-clients
Expose first-class `CHAR(n)` and `VARCHAR(n)` types and lengths through SQL client metadata when `spark.sql.charVarchar.standardSemantics.enabled` is on:
- Spark Connect JDBC maps `CharType` / `VarcharType` to `java.sql.Types.CHAR` / `VARCHAR`, with Java string values and precision/display size `n`.
- HiveServer2 `DatabaseMetaData.getColumns` reports `COLUMN_SIZE = n` for `VARCHAR(n)`, matching its existing `CHAR(n)` behavior.
- HiveServer2 result metadata coverage verifies `Types.CHAR` / `Types.VARCHAR` and precision `n` for first-class CAST results.
The existing Connect proto already carries CHAR/VARCHAR and their lengths; the Connect JDBC test exercises that schema round trip as well as value collection.
### Why are the changes needed?
Spark Connect JDBC currently rejects first-class `CharType` and `VarcharType` as unsupported because its metadata mapping only recognizes the `StringType` singleton. HiveServer2 already identifies VARCHAR catalog columns correctly but reports their `COLUMN_SIZE` as unknown (`0`). Clients therefore cannot reliably describe or size CHAR/VARCHAR columns even though Catalyst retains the type and length.
### Does this PR introduce _any_ user-facing change?
Yes, when first-class CHAR/VARCHAR types are present: JDBC and HiveServer2 metadata now report the corresponding JDBC type and declared character length instead of rejecting the type or reporting an unknown size.
### How was this patch tested?
- Added `SparkConnectJdbcDataTypeSuite` coverage for CHAR/VARCHAR values, JDBC types, class names, signedness, precision, scale, and display size.
- Added `SparkMetadataOperationSuite` result metadata coverage and updated `getColumns` expectations for `VARCHAR(n)`.
- Both changed test suites compile. Local execution was blocked by unrelated build-environment failures: the HiveServer2 child process hit a stale `ConfigBuilder.withBindingPolicy` linkage error, and the Connect build could not resolve `com.google.errorprone.annotations.CheckReturnValue` while compiling `connect-common`.
### Was this patch authored or co-authored using generative AI tooling?
Yes (Cursor).
--
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-58794][SQL] Expose CHAR/VARCHAR metadata to SQL clients [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3817597423
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkResult.scala:
##########
@@ -101,7 +101,7 @@ private[sql] class SparkResult[T](
case UnboundRowEncoder =>
// Replace the row encoder with the encoder inferred from the schema.
RowEncoder
- .encoderFor(dataType.asInstanceOf[StructType])
+ .encoderForResultSchema(dataType.asInstanceOf[StructType])
Review Comment:
Correct call site: only `UnboundRowEncoder` (row `collect`) should ignore local CHAR/VARCHAR config. Typed encoders stay as-is and still decode Arrow string vectors via `StringEncoder`.
Worth a sentence in the PR description: without this, the JDBC CAST collect assertions in this PR would fail on a client that only has `DefaultSqlApiConf`.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -72,14 +72,31 @@ object RowEncoder extends DataTypeErrorsBase {
encoderForDataType(schema, lenient).asInstanceOf[AgnosticEncoder[Row]]
}
+ /**
+ * Builds an encoder for a schema that the engine produced, such as the result schema of a Spark
+ * Connect query. Whether CHAR/VARCHAR are first class types is decided by the session that
+ * produced the schema, so they are always accepted here. A client cannot read that session's
+ * configuration, and refusing the type would make the result undecodable.
+ */
+ private[sql] def encoderForResultSchema(schema: StructType): AgnosticEncoder[Row] =
Review Comment:
This is the right verb: engine-produced schema vs user-supplied schema, instead of reading a session conf the client does not have.
Please add a negative test that `encoderFor` (not this method) still rejects `CharType` / `VarcharType` when `charVarcharFirstClassTypes` is false, so UDF / `createDataFrame` cannot pick up the result-schema override.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/arrow/ArrowDeserializer.scala:
##########
@@ -135,8 +135,14 @@ object ArrowDeserializers {
new Deserializer[Any] {
def get(i: Int): Any = null
}
- case (StringEncoder, v: FieldVector) =>
- new LeafFieldDeserializer[String](encoder, v, timeZoneId) {
+ // CHAR/VARCHAR travel as plain Arrow string vectors; the length is part of the type, not of
+ // the encoding, and the values arrive already padded and length checked by the server. Read
+ // them against the unconstrained string type, since narrowing STRING to CHAR(n)/VARCHAR(n)
+ // is not an up-cast and the reader would reject the vector.
+ case (StringEncoder | _: CharEncoder | _: VarcharEncoder, v: FieldVector) =>
Review Comment:
Reading CHAR/VARCHAR through `plainStringType` is the right explanation (Arrow has no CHAR vector; narrowing is not an up-cast). Please keep the comment; it is load-bearing.
The serializer change is needed for the ArrowEncoderSuite round trip. Production `SparkResult` is decode-only, so skipping write-side length checks here is acceptable -- just do not start using `encoderForResultSchema` to encode user input.
--
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-58806][SQL][CONNECT] Expose CHAR/VARCHAR metadata and decode result schemas [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3819859451
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetColumnsOperation.scala:
##########
@@ -126,16 +126,18 @@ private[hive] class SparkGetColumnsOperation(
}
/**
- * For boolean, numeric and datetime types, it returns the default size of its catalyst type
+ * For boolean, numeric and datetime types, it returns the default size of its catalyst type.
Review Comment:
**Nit:**
The pronoun reads as referring to the method, which has no Catalyst type.
```suggestion
* For boolean, numeric and datetime types, this method returns the input type's default size.
```
--
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-58806][SQL][CONNECT] Expose CHAR/VARCHAR metadata and decode result schemas [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3818586624
##########
sql/connect/client/jdbc/src/main/scala/org/apache/spark/sql/connect/client/jdbc/util/JdbcTypeUtils.scala:
##########
@@ -168,6 +172,17 @@ private[jdbc] object JdbcTypeUtils {
case _ => null
}
+ /**
+ * JDBC `CHAR_OCTET_LENGTH`. Spark CHAR/VARCHAR lengths are in characters; report `n` so
+ * clients that size buffers from this column see the declared width instead of 0.
+ * Unbounded STRING and non-character types keep 0 (not applicable / unknown).
+ */
+ def getCharOctetLength(field: StructField): Int = field.dataType match {
+ case c: CharType => c.length
Review Comment:
Agreed -- JDBC `CHAR_OCTET_LENGTH` is a byte capacity. Spark `n` is characters, so both Connect and HiveServer2 now report `4 * n` (UTF-8 maximum bytes per character), saturating at `Int.MaxValue`. Unbounded STRING stays 0 / null.
Tests: Connect `CHAR(4)` / `VARCHAR(6)` assert 16 / 24; HS2 asserts `length * 4`.
`3e5a346`.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -72,14 +72,31 @@ object RowEncoder extends DataTypeErrorsBase {
encoderForDataType(schema, lenient).asInstanceOf[AgnosticEncoder[Row]]
}
+ /**
+ * Builds an encoder for a schema that the engine produced, such as the result schema of a Spark
+ * Connect query. Whether CHAR/VARCHAR are first class types is decided by the session that
Review Comment:
Applied in `3e5a346`.
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetColumnsOperation.scala:
##########
@@ -126,16 +126,18 @@ private[hive] class SparkGetColumnsOperation(
}
/**
- * For boolean, numeric and datetime types, it returns the default size of its catalyst type
+ * For boolean, numeric and datetime types, it returns the default size of its catalyst type.
+ * For CHAR(n) and VARCHAR(n), it returns the declared character length n.
* For struct type, when its elements are fixed-size, the summation of all element sizes will be
* returned.
- * For array, map, string, and binaries, the column size is variable, return null as unknown.
+ * For array, map, unbounded string, and binaries, the column size is variable, return null.
Review Comment:
Applied in `3e5a346`.
--
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-58794][SQL] Expose CHAR/VARCHAR metadata to SQL clients [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3815985442
##########
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:
Addressed in 4673559.
`CHAR_OCTET_LENGTH` now reports the declared length `n` for CHAR/VARCHAR (STRING stays 0). `getTypeInfo` lists CHAR and VARCHAR with `CREATE_PARAMS = length`, ordered by DATA_TYPE then TYPE_NAME. Added a table `getColumns` test covering type code, type name, COLUMN_SIZE, and CHAR_OCTET_LENGTH.
##########
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:
Added `SPARK-58794: getColumns for CHAR/VARCHAR table` in this suite. Connect JDBC suites still fail to compile locally (`CheckReturnValue`); relying on GitHub CI for the run.
##########
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:
Scaladoc updated. HS2 `CHAR_OCTET_LENGTH` now returns `n` for CHAR/VARCHAR and stays null otherwise, matching Connect's filled-in width (Connect uses 0 for N/A).
##########
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:
Matching `CharType` / `VarcharType` on `.length` now, with explicit 255 / 1024 for c17 / c18. The table test also enables `standardSemantics`.
##########
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:
c17 / c18 `COLUMN_SIZE` (and `CHAR_OCTET_LENGTH`) are asserted in the `getColumns` test above. This CAST result-metadata test stays as the Hive JDBC name pin (`char` / `varchar`).
--
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-58794][SQL][CONNECT] Expose CHAR/VARCHAR metadata and decode result schemas [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3817636520
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -72,14 +72,31 @@ object RowEncoder extends DataTypeErrorsBase {
encoderForDataType(schema, lenient).asInstanceOf[AgnosticEncoder[Row]]
}
+ /**
+ * Builds an encoder for a schema that the engine produced, such as the result schema of a Spark
+ * Connect query. Whether CHAR/VARCHAR are first class types is decided by the session that
+ * produced the schema, so they are always accepted here. A client cannot read that session's
+ * configuration, and refusing the type would make the result undecodable.
+ */
+ private[sql] def encoderForResultSchema(schema: StructType): AgnosticEncoder[Row] =
Review Comment:
Added `RowEncoderSuite` "SPARK-58794: encoderFor rejects CHAR/VARCHAR when first-class types are off". With both `CHAR_VARCHAR_STANDARD_SEMANTICS` and `PRESERVE_CHAR_VARCHAR_TYPE_INFO` off, `RowEncoder.encoderFor` still raises `UNSUPPORTED_DATA_TYPE_FOR_ENCODER`; `encoderForResultSchema` accepts the same schema so engine-produced Connect results remain decodable.
`7f6e4136ef5`.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/SparkResult.scala:
##########
@@ -101,7 +101,7 @@ private[sql] class SparkResult[T](
case UnboundRowEncoder =>
// Replace the row encoder with the encoder inferred from the schema.
RowEncoder
- .encoderFor(dataType.asInstanceOf[StructType])
+ .encoderForResultSchema(dataType.asInstanceOf[StructType])
Review Comment:
Kept `encoderForResultSchema` as the result-schema entry point. `SparkResult.createEncoder` uses it only for `UnboundRowEncoder`; other encoder bindings still go through `encoderFor`. The PR description now covers that split, the Arrow STRING up-cast, and why JDBC CAST collect needed it.
Architecture of the new entry point is unchanged.
##########
sql/connect/common/src/main/scala/org/apache/spark/sql/connect/client/arrow/ArrowDeserializer.scala:
##########
@@ -135,8 +135,14 @@ object ArrowDeserializers {
new Deserializer[Any] {
def get(i: Int): Any = null
}
- case (StringEncoder, v: FieldVector) =>
- new LeafFieldDeserializer[String](encoder, v, timeZoneId) {
+ // CHAR/VARCHAR travel as plain Arrow string vectors; the length is part of the type, not of
+ // the encoding, and the values arrive already padded and length checked by the server. Read
+ // them against the unconstrained string type, since narrowing STRING to CHAR(n)/VARCHAR(n)
+ // is not an up-cast and the reader would reject the vector.
+ case (StringEncoder | _: CharEncoder | _: VarcharEncoder, v: FieldVector) =>
Review Comment:
Left the Arrow deserializer `plainStringType` comment as-is. Python Connect already maps proto `char` / `var_char` in `pyspark.sql.connect.types`; added `SparkConnectBasicTests.test_char_varchar_result_schema` (classic vs Connect schema + `collect()` with standard semantics).
`CHAR_OCTET_LENGTH` stays character `n` (documented on `JdbcTypeUtils.getCharOctetLength` and in the PR description). Connect and HS2 `getColumns` tests assert `=== n`; that is the CI gate rather than a semantic change.
--
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-58806][SQL][CONNECT] Expose CHAR/VARCHAR metadata and decode result schemas [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #58132: [SPARK-58806][SQL][CONNECT] Expose CHAR/VARCHAR metadata and decode result schemas
URL: https://github.com/apache/spark/pull/58132
--
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-58794][SQL] Expose CHAR/VARCHAR metadata to SQL clients [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
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: 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-58806][SQL][CONNECT] Expose CHAR/VARCHAR metadata and decode result schemas [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #58132:
URL: https://github.com/apache/spark/pull/58132#issuecomment-5357445947
**Merge Summary:**
- merged into master https://github.com/apache/spark/commit/6af294ef12f13ed5c6cb0f207e4b29d8d407e337
- merged into branch-4.x https://github.com/apache/spark/commit/f48d84f0b8b51894a996204dc69a808dbaf9ea4c
*Posted by `merge_spark_pr.py`*
--
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-58794][SQL] Expose CHAR/VARCHAR metadata to SQL clients [spark]
Posted by "srielau (via GitHub)" <gi...@apache.org>.
srielau commented on PR #58132:
URL: https://github.com/apache/spark/pull/58132#issuecomment-5347036878
Re-review remaining items:
1. **CI** -- not green yet. Fork Build for `4673559` is queued/in progress: https://github.com/srielau/spark/actions/runs/32291730651 (`Check changes` passed; remaining jobs still queued). The earlier Build on this branch was cancelled by the follow-up push. Local runs of `SparkConnectJdbcDataTypeSuite`, `SparkConnectDatabaseMetaDataSuite`, and `SparkMetadataOperationSuite` are still blocked here. Those three suites remain the merge gate.
2. **PR description** -- refreshed: `CHAR_OCTET_LENGTH`, `getTypeInfo` CHAR/VARCHAR rows, table `getColumns`, and the How-tested list now match `4673559`.
3. **Octet length vs UTF-8 bytes** -- called out in What/Why/user-facing. Helpers already document that Spark reports character length `n` (Connect 0 / HS2 null for unbounded STRING). No production change.
--
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-58794][SQL][CONNECT] Expose CHAR/VARCHAR metadata and decode result schemas [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #58132:
URL: https://github.com/apache/spark/pull/58132#discussion_r3818524418
##########
sql/connect/client/jdbc/src/main/scala/org/apache/spark/sql/connect/client/jdbc/util/JdbcTypeUtils.scala:
##########
@@ -168,6 +172,17 @@ private[jdbc] object JdbcTypeUtils {
case _ => null
}
+ /**
+ * JDBC `CHAR_OCTET_LENGTH`. Spark CHAR/VARCHAR lengths are in characters; report `n` so
+ * clients that size buffers from this column see the declared width instead of 0.
+ * Unbounded STRING and non-character types keep 0 (not applicable / unknown).
+ */
+ def getCharOctetLength(field: StructField): Int = field.dataType match {
+ case c: CharType => c.length
Review Comment:
**Blocking:**
`CHAR_OCTET_LENGTH` is byte capacity, but this returns character count. A `CHAR(4)` value can contain four multibyte characters, so clients sizing a buffer from this value can truncate it. Please report the maximum UTF-8 byte length with overflow handling, or leave the field unknown; apply the same correction to the HiveServer2 helper.
##########
sql/api/src/main/scala/org/apache/spark/sql/catalyst/encoders/RowEncoder.scala:
##########
@@ -72,14 +72,31 @@ object RowEncoder extends DataTypeErrorsBase {
encoderForDataType(schema, lenient).asInstanceOf[AgnosticEncoder[Row]]
}
+ /**
+ * Builds an encoder for a schema that the engine produced, such as the result schema of a Spark
+ * Connect query. Whether CHAR/VARCHAR are first class types is decided by the session that
Review Comment:
**Nit:**
```suggestion
* Connect query. Whether CHAR/VARCHAR are first-class types is decided by the session that
```
##########
sql/hive-thriftserver/src/main/scala/org/apache/spark/sql/hive/thriftserver/SparkGetColumnsOperation.scala:
##########
@@ -126,16 +126,18 @@ private[hive] class SparkGetColumnsOperation(
}
/**
- * For boolean, numeric and datetime types, it returns the default size of its catalyst type
+ * For boolean, numeric and datetime types, it returns the default size of its catalyst type.
+ * For CHAR(n) and VARCHAR(n), it returns the declared character length n.
* For struct type, when its elements are fixed-size, the summation of all element sizes will be
* returned.
- * For array, map, string, and binaries, the column size is variable, return null as unknown.
+ * For array, map, unbounded string, and binaries, the column size is variable, return null.
Review Comment:
**Nit:**
```suggestion
* For array, map, unbounded string, and binaries, the column size is variable; return null.
```
--
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