andygrove commented on code in PR #5051:
URL: https://github.com/apache/datafusion-comet/pull/5051#discussion_r3693347774
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/CometArrowConverters.scala:
##########
@@ -74,4 +74,45 @@ object CometArrowConverters extends Logging {
}
}
}
+
+ /**
+ * Copy a Spark `ColumnarBatch` whose columns are not Arrow-backed (e.g.
+ * `On/OffHeapColumnVector` from Spark's vectorized Parquet reader, or a
third-party connector's
+ * vectors) into a freshly allocated Arrow `ColumnarBatch` of `CometVector`s.
+ *
+ * The input batch is not consumed or closed; the caller owns the returned
batch and must close
+ * it. Values are copied element-wise, since Spark's `ColumnVector`
implementations do not
+ * expose Arrow buffers.
+ */
+ def columnarBatchToArrowBatch(
Review Comment:
Thanks for digging into Spark's implementations — that answers it, and I
have taken your recommendation in c25c7b32f.
Your framing of the difference is the key point and I had missed it: Spark's
Arrow cache can construct `TimestampType` from `conf.sessionLocalTimeZone`
safely *because* its RecordBatch is schema-less, so the label is never
persisted and gets reconstructed on read. `CometCachedBatch` stores a full IPC
stream including the schema, so for us that label really is written down —
which makes using a mutable session value materially worse than it looks by
analogy with Spark.
So the row path now passes `CometArrowStream.NATIVE_TIMEZONE`, matching the
columnar path, which already did. Confirmed the two things you predicted:
- **No values move.** Spark's internal representation is micros since the
Unix epoch regardless of session timezone, so this is a label-only change. The
test checks both the values and their `CAST(ts AS STRING)` rendering against
Spark under two non-UTC session timezones.
- **`TimestampNTZType` needs nothing.** `Utils.toArrowType` maps it to
`Timestamp(MICROSECOND, null)` whatever timezone is passed, so it was already
timezone-free.
A small bonus: the closure no longer needs the session timezone, so it no
longer captures anything derived from `conf` at all. Worth noting because the
original had to hoist `val sessionTz` outside `mapPartitions` — when I moved
the read inside while testing, it NPEd in `ConfigEntry.readString` on the
executor. That hazard is now gone.
The test caches a row-based plan (local `Seq`, so
`convertInternalRowToCachedBatch` rather than the columnar path) under
`America/Los_Angeles` and `Asia/Kolkata`, decodes the cached batches back
through the serializer, and asserts the Arrow field's timezone. I checked it is
not vacuous: reverting the one-line change fails it with `got
[America/Los_Angeles]`.
##########
spark/src/main/scala/org/apache/spark/sql/comet/execution/arrow/CometArrowConverters.scala:
##########
@@ -74,4 +74,45 @@ object CometArrowConverters extends Logging {
}
}
}
+
+ /**
+ * Copy a Spark `ColumnarBatch` whose columns are not Arrow-backed (e.g.
+ * `On/OffHeapColumnVector` from Spark's vectorized Parquet reader, or a
third-party connector's
+ * vectors) into a freshly allocated Arrow `ColumnarBatch` of `CometVector`s.
+ *
+ * The input batch is not consumed or closed; the caller owns the returned
batch and must close
+ * it. Values are copied element-wise, since Spark's `ColumnVector`
implementations do not
+ * expose Arrow buffers.
+ */
+ def columnarBatchToArrowBatch(
Review Comment:
Agreed, and that is what I ended up doing —
`SparkColumnarArrowReader.loadNextBatch` and `columnarBatchToArrowBatch` now
share `CometArrowConverters.writeColumns` (c7bc49e5), so there is one copy loop
rather than two. Marking this thread as settled; the remaining timezone
question from it is answered on the sibling thread.
--
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]
