You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "zhangxinyao88 (via GitHub)" <gi...@apache.org> on 2026/10/06 03:55:31 UTC
[PR] [SPARK-57744][SQL] Route vectorized column population and row getters through type operations [spark]
zhangxinyao88 opened a new pull request, #59249:
URL: https://github.com/apache/spark/pull/59249
### What changes were proposed in this pull request?
Route `ColumnarRow.get` and `ColumnarBatchRow.get` through `SpecializedGettersReader`, preserving each wrapper's null-handling contract. Route framework-registered values in `ColumnVectorUtils.toBatch` through `TypeOps.toCatalyst` and append according to their physical representation. Preserve support for internal `TimestampNanosVal` inputs.
### Why are the changes needed?
The row getters maintain separate data-type lists and reject TIME despite its supported long representation. The column-population path similarly duplicates type-specific conversion logic. Using the shared operations lets the existing framework registrations handle TIME and nanosecond timestamps consistently.
### Does this PR introduce _any_ user-facing change?
Yes, in vectorized row utilities on the development branch:
- Generic row getters can read TIME. `ColumnarRow.get` also gains calendar interval and geospatial binary-view support from the shared reader.
- `toBatch` accepts the framework's external nanosecond timestamp values (`LocalDateTime` for NTZ and `Instant` for LTZ), honoring precision, while continuing to accept internal nanos values unchanged.
- Unsupported getter types use `_LEGACY_ERROR_TEMP_3131` instead of `_LEGACY_ERROR_TEMP_3152` or `_LEGACY_ERROR_TEMP_3155`.
### How was this patch tested?
The new generic TIME assertion failed at precisions 0, 6, 7, and 9 before the production change. After the change, both `ColumnarBatchSuite` and `ColumnVectorUtilsSuite` passed: 107 tests, zero failures. Added coverage includes nested TIME reads, nullable struct fields, interval reads, external timestamp precision, internal timestamp inputs, and null timestamps in both memory modes. Existing UDT and randomized conversion tests also passed.
Local SQL Core test compilation has unrelated Parquet test-helper errors, so the run selected only these suites and their `RandomDataGenerator` helper:
```sh
build/sbt 'project sql' \
'set Test / sources := (Test / sources).value.filter(f => Set("ColumnarBatchSuite.scala", "ColumnVectorUtilsSuite.scala", "RandomDataGenerator.scala").contains(f.getName))' \
'testOnly *ColumnarBatchSuite *ColumnVectorUtilsSuite'
```
Catalyst and SQL Core Java Checkstyle passed. SQL Core test Scalastyle passed with zero errors and warnings. `git diff --check` passed.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: OpenAI Codex
--
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-57744][SQL] Route vectorized column population and row getters through type operations [spark]
Posted by "zhangxinyao88 (via GitHub)" <gi...@apache.org>.
zhangxinyao88 commented on code in PR #59249:
URL: https://github.com/apache/spark/pull/59249#discussion_r4235817397
##########
sql/catalyst/src/main/java/org/apache/spark/sql/vectorized/ColumnarBatchRow.java:
##########
@@ -185,52 +184,7 @@ public ColumnarMap getMap(int ordinal) {
@Override
public Object get(int ordinal, DataType dataType) {
- if (dataType instanceof BooleanType) {
- return getBoolean(ordinal);
- } else if (dataType instanceof ByteType) {
- return getByte(ordinal);
- } else if (dataType instanceof ShortType) {
- return getShort(ordinal);
- } else if (dataType instanceof IntegerType || dataType instanceof YearMonthIntervalType) {
- return getInt(ordinal);
- } else if (dataType instanceof LongType || dataType instanceof DayTimeIntervalType) {
- return getLong(ordinal);
- } else if (dataType instanceof FloatType) {
- return getFloat(ordinal);
- } else if (dataType instanceof DoubleType) {
- return getDouble(ordinal);
- } else if (dataType instanceof StringType) {
- return getUTF8String(ordinal);
- } else if (dataType instanceof BinaryType) {
- return getBinary(ordinal);
- } else if (dataType instanceof GeographyType || dataType instanceof GeometryType) {
- return getBinaryView(ordinal);
- } else if (dataType instanceof DecimalType t) {
- return getDecimal(ordinal, t.precision(), t.scale());
- } else if (dataType instanceof DateType) {
- return getInt(ordinal);
- } else if (dataType instanceof TimestampType) {
- return getLong(ordinal);
- } else if (dataType instanceof TimestampNTZType) {
- return getLong(ordinal);
- } else if (dataType instanceof TimestampNTZNanosType) {
- return getTimestampNTZNanos(ordinal);
- } else if (dataType instanceof TimestampLTZNanosType) {
- return getTimestampLTZNanos(ordinal);
- } else if (dataType instanceof ArrayType) {
- return getArray(ordinal);
- } else if (dataType instanceof StructType structType) {
- return getStruct(ordinal, structType.fields().length);
- } else if (dataType instanceof MapType) {
- return getMap(ordinal);
- } else if (dataType instanceof VariantType) {
- return getVariant(ordinal);
- } else if (dataType instanceof UserDefinedType<?> udt) {
- return get(ordinal, udt.sqlType());
- } else {
- throw new SparkUnsupportedOperationException(
- "_LEGACY_ERROR_TEMP_3152", Map.of("dataType", String.valueOf(dataType)));
Review Comment:
> Could you delete both entries in this PR so they don't need a separate cleanup later (as was done for `_LEGACY_ERROR_TEMP_2088` in the SPARK-48803 follow-up)?
Removed both entries in dafd1a75c5.
--
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