You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "MaxGekk (via GitHub)" <gi...@apache.org> on 2026/05/29 09:03:28 UTC
[PR] [SPARK-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
MaxGekk opened a new pull request, #56198:
URL: https://github.com/apache/spark/pull/56198
### What changes were proposed in this pull request?
Implement columnar storage support for `TimestampNTZNanosType` and `TimestampLTZNanosType` across the column-vector stack. The layout mirrors `CalendarInterval`: each column gets two child vectors — a `Long` child for `epochMicros` and a `Short` child for `nanosWithinMicro` (range [0, 999]).
Concretely:
- **`ColumnVector`** — `getTimestampNTZNanos` / `getTimestampLTZNanos` now read from child vectors instead of throwing `SparkUnsupportedOperationException`.
- **`WritableColumnVector`** — allocates the two child columns in the constructor; adds `putTimestampNTZNanos` / `putTimestampLTZNanos` write methods.
- **`ConstantColumnVector`** — same child-column allocation; adds `setTimestampNanosVal` for the constant-value (partition-column) path.
- **`RowToColumnConverter` (`Columnar.scala`)** — adds `TimestampNTZNanosConverter` / `TimestampLTZNanosConverter` objects (append `epochMicros` + `nanosWithinMicro` to children via `appendStruct`); routes nullable columns through `StructNullableTypeConverter`.
- **`ColumnVectorUtils`** — handles both types in `populate` (constant-column path) and in `appendValue` (null and non-null branches).
### Why are the changes needed?
SPARK-56981 added row-level physical representation for nanosecond timestamps, but columnar execution could not hold or move these values — any attempt to build a `ColumnarBatch` from rows containing nanosecond timestamps threw an unsupported-operation exception. This PR closes that gap.
### Does this PR introduce _any_ user-facing change?
Yes. `ColumnarBatch` can now be built from `InternalRow`s containing `TimestampNTZNanosType` / `TimestampLTZNanosType` values. Previously this threw `SparkUnsupportedOperationException`.
### How was this patch tested?
Added four unit tests to `RowToColumnConverterSuite`:
- `TimestampNTZNanosType column roundtrip` — non-null values survive the row→column→read cycle.
- `TimestampNTZNanosType column with nulls` — null slots are preserved correctly.
- `TimestampLTZNanosType column roundtrip` — same for the LTZ variant.
- `TimestampLTZNanosType column with nulls` — same for the LTZ variant.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Sonnet 4.6 (claude.ai/code)
--
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-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56198:
URL: https://github.com/apache/spark/pull/56198#discussion_r3330860677
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -106,6 +107,10 @@ public static void populate(
} else if (pdt instanceof PhysicalCalendarIntervalType) {
// The value of `numRows` is irrelevant.
col.setCalendarInterval((CalendarInterval) row.get(fieldIdx, t));
+ } else if (pdt instanceof PhysicalTimestampNTZNanosType) {
Review Comment:
Done. Collapsed both nanos branches into one condition with `||`, matching `appendValue`.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnVectorSuite.scala:
##########
@@ -379,6 +379,50 @@ class ColumnVectorSuite extends SparkFunSuite with SQLHelper {
}
}
+ testVectors("timestamp_ntz_nanos", 10, TimestampNTZNanosType(9)) { testVector =>
+ val values = (0 until 10).map(i => TimestampNanosVal.fromParts(i * 1000L, i.toShort))
+ values.foreach { v =>
+ testVector.putNotNull(testVector.elementsAppended)
+ testVector.putTimestampNTZNanos(testVector.elementsAppended, v)
+ testVector.elementsAppended += 1
+ }
+ values.zipWithIndex.foreach { case (v, i) =>
+ assert(testVector.getTimestampNTZNanos(i) === v)
+ }
+ testVector.putNull(0)
+ assert(testVector.isNullAt(0))
+ }
+
+ testVectors("timestamp_ltz_nanos", 10, TimestampLTZNanosType(9)) { testVector =>
+ val values = (0 until 10).map(i => TimestampNanosVal.fromParts(i * 1000L, i.toShort))
+ values.foreach { v =>
+ testVector.putNotNull(testVector.elementsAppended)
+ testVector.putTimestampLTZNanos(testVector.elementsAppended, v)
+ testVector.elementsAppended += 1
+ }
+ values.zipWithIndex.foreach { case (v, i) =>
+ assert(testVector.getTimestampLTZNanos(i) === v)
+ }
+ testVector.putNull(0)
+ assert(testVector.isNullAt(0))
+ }
+
+ testVectors("mutable ColumnarRow with TimestampNTZNanosType", 5,
+ TimestampNTZNanosType(9)) { testVector =>
+ val mutableRow = new MutableColumnarRow(Array(testVector))
+ val values = (0 until 5).map(i => TimestampNanosVal.fromParts(i * 100L, i.toShort))
+ values.zipWithIndex.foreach { case (v, i) =>
+ mutableRow.rowId = i
+ mutableRow.setTimestampNTZNanos(0, v)
+ }
+ values.zipWithIndex.foreach { case (v, i) =>
+ mutableRow.rowId = i
+ assert(mutableRow.getTimestampNTZNanos(0) === v)
+ assert(mutableRow.get(0, TimestampNTZNanosType(9)) === v)
+ assert(mutableRow.copy().get(0, TimestampNTZNanosType(9)) === v)
+ }
+ }
Review Comment:
Done. Added a parallel `mutable ColumnarRow with TimestampLTZNanosType` test that exercises `setTimestampLTZNanos`/`getTimestampLTZNanos`, the `update`/`get` dispatches, and the `copy()` branch.
--
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-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56198:
URL: https://github.com/apache/spark/pull/56198#issuecomment-4588202406
Merging to master/4.x. Thank you @peter-toth for review.
--
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-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on code in PR #56198:
URL: https://github.com/apache/spark/pull/56198#discussion_r3328503541
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java:
##########
@@ -751,7 +762,10 @@ public final int appendStruct(boolean isNull) {
putNull(elementsAppended);
elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType || c.type instanceof VariantType) {
+ if (c.type instanceof StructType || c.type instanceof VariantType
+ || c.type instanceof CalendarIntervalType
Review Comment:
Adding `CalendarIntervalType` here isn't just supporting the new nanos types — it also fixes a previously-latent bug for nested struct-of-interval. Pre-PR, when an outer struct column was appended as null and one of its child columns was a `CalendarInterval`, the interval child took the `else` branch (`c.appendNull()`), advancing only the interval's own cursor and leaving its three grandchild primitive columns (`months`/`days`/`microseconds`) un-advanced. Subsequent rows would then write into the wrong grandchild slots — silent skew.
The fix is correct, but:
1. The PR description doesn't mention this. Worth one line so reviewers don't miss the interval-semantics change.
2. There's no test exercising the new recursion. The minimum case is a struct-of-interval column with at least one null parent row, then read back the next non-null row's children to verify they aren't shifted. Same shape extends to struct-of-`TimestampNanos`.
Up to you whether to split out into a separate commit or keep bundled.
##########
sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnVectorSuite.scala:
##########
@@ -379,6 +379,50 @@ class ColumnVectorSuite extends SparkFunSuite with SQLHelper {
}
}
+ testVectors("timestamp_ntz_nanos", 10, TimestampNTZNanosType(9)) { testVector =>
+ val values = (0 until 10).map(i => TimestampNanosVal.fromParts(i * 1000L, i.toShort))
+ values.foreach { v =>
+ testVector.putNotNull(testVector.elementsAppended)
+ testVector.putTimestampNTZNanos(testVector.elementsAppended, v)
+ testVector.elementsAppended += 1
+ }
+ values.zipWithIndex.foreach { case (v, i) =>
+ assert(testVector.getTimestampNTZNanos(i) === v)
+ }
+ testVector.putNull(0)
+ assert(testVector.isNullAt(0))
+ }
+
+ testVectors("timestamp_ltz_nanos", 10, TimestampLTZNanosType(9)) { testVector =>
+ val values = (0 until 10).map(i => TimestampNanosVal.fromParts(i * 1000L, i.toShort))
+ values.foreach { v =>
+ testVector.putNotNull(testVector.elementsAppended)
+ testVector.putTimestampLTZNanos(testVector.elementsAppended, v)
+ testVector.elementsAppended += 1
+ }
+ values.zipWithIndex.foreach { case (v, i) =>
+ assert(testVector.getTimestampLTZNanos(i) === v)
+ }
+ testVector.putNull(0)
+ assert(testVector.isNullAt(0))
+ }
+
+ testVectors("mutable ColumnarRow with TimestampNTZNanosType", 5,
+ TimestampNTZNanosType(9)) { testVector =>
+ val mutableRow = new MutableColumnarRow(Array(testVector))
+ val values = (0 until 5).map(i => TimestampNanosVal.fromParts(i * 100L, i.toShort))
+ values.zipWithIndex.foreach { case (v, i) =>
+ mutableRow.rowId = i
+ mutableRow.setTimestampNTZNanos(0, v)
+ }
+ values.zipWithIndex.foreach { case (v, i) =>
+ mutableRow.rowId = i
+ assert(mutableRow.getTimestampNTZNanos(0) === v)
+ assert(mutableRow.get(0, TimestampNTZNanosType(9)) === v)
+ assert(mutableRow.copy().get(0, TimestampNTZNanosType(9)) === v)
+ }
+ }
Review Comment:
The PR adds a `MutableColumnarRow` test for `TimestampNTZNanosType` but not for `TimestampLTZNanosType`. The LTZ paths (`setTimestampLTZNanos` at `MutableColumnarRow.java:348`, the `update(TimestampLTZNanosType)` and `get(TimestampLTZNanosType)` dispatches at `MutableColumnarRow.java:240,272`, and the `copy()` branch at `MutableColumnarRow.java:104`) aren't exercised. Adding a parallel `mutable ColumnarRow with TimestampLTZNanosType` block right after this one closes the gap.
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVectorUtils.java:
##########
@@ -106,6 +107,10 @@ public static void populate(
} else if (pdt instanceof PhysicalCalendarIntervalType) {
// The value of `numRows` is irrelevant.
col.setCalendarInterval((CalendarInterval) row.get(fieldIdx, t));
+ } else if (pdt instanceof PhysicalTimestampNTZNanosType) {
Review Comment:
The two `else if` branches are identical. `appendValue` below at line 178 already collapses both nanos types into one condition with `||`. Suggest the same here:
```java
} else if (pdt instanceof PhysicalTimestampNTZNanosType ||
pdt instanceof PhysicalTimestampLTZNanosType) {
col.setTimestampNanosVal((TimestampNanosVal) row.get(fieldIdx, t));
}
```
--
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-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on PR #56198:
URL: https://github.com/apache/spark/pull/56198#issuecomment-4581801376
@dongjoon-hyun @peter-toth May I ask you to review this PR. We need this to support timestamps with nanosecond precision in Parquet vectorized reader and in ORC.
--
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-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk commented on code in PR #56198:
URL: https://github.com/apache/spark/pull/56198#discussion_r3330855271
##########
sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/WritableColumnVector.java:
##########
@@ -751,7 +762,10 @@ public final int appendStruct(boolean isNull) {
putNull(elementsAppended);
elementsAppended++;
for (WritableColumnVector c: childColumns) {
- if (c.type instanceof StructType || c.type instanceof VariantType) {
+ if (c.type instanceof StructType || c.type instanceof VariantType
+ || c.type instanceof CalendarIntervalType
Review Comment:
I've extracted the `CalendarIntervalType` fix into a separate PR, along with a test that covers the struct-of-interval null-parent skew case: apache/spark#56235 (SPARK-57184). This PR now only adds the nanosecond timestamp types to the `appendStruct` recursion guard.
--
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-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
Posted by "peter-toth (via GitHub)" <gi...@apache.org>.
peter-toth commented on PR #56198:
URL: https://github.com/apache/spark/pull/56198#issuecomment-4582407175
@MaxGekk, probably we should extract the `CalendarIntervalType` related fix to a separate PR, but the other 2 are just minor nits.
--
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-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types [spark]
Posted by "MaxGekk (via GitHub)" <gi...@apache.org>.
MaxGekk closed pull request #56198: [SPARK-57100][SQL] Add columnar (ColumnVector) support for nanosecond timestamp types
URL: https://github.com/apache/spark/pull/56198
--
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