peterxcli commented on code in PR #5051:
URL: https://github.com/apache/datafusion-comet/pull/5051#discussion_r3679301588
##########
spark/src/main/scala/org/apache/spark/sql/comet/util/Utils.scala:
##########
@@ -397,6 +397,15 @@ object Utils extends CometTypeShim with Logging {
}
}
+ /**
+ * Whether every column in `batch` satisfies [[getBatchFieldVectors]]'s
precondition, i.e. is an
+ * Arrow-backed `CometVector`. Callers that may receive batches from a plan
they did not build
+ * (e.g. Comet's cache serializer, which Spark hands the cached plan's
columnar output) use this
+ * to convert foreign vectors to Arrow instead of tripping the exception
below.
+ */
+ def isArrowBacked(batch: ColumnarBatch): Boolean =
Review Comment:
Is this possible to be false?
##########
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:
> Do you have a preference for which one the cache format should standardise
on?
Good catch. I checked Spark’s cache implementations. Released Spark’s
default cache does not store timezone metadata: both `TimestampType` and
`TimestampNTZType` are stored as raw longs, with LTZ versus NTZ preserved by
the relation schema.
Current Spark's Arrow cache constructs `TimestampType` using
`conf.sessionLocalTimeZone` and `TimestampNTZType` with `timezone=null`, but
its cached RecordBatch is deliberately schema-less. The timezone label is
therefore not persisted and is reconstructed on read.
`CometCachedBatch` differs because it stores a complete IPC stream including
the schema. I would therefore standardize its physical format on
`CometArrowStream.NATIVE_TIMEZONE` (`UTC`) for `TimestampType`, while keeping
`TimestampNTZType` timezone-free. This avoids embedding the write session’s
mutable timezone, matches Comet’s native schema, and avoids an unnecessary
native-boundary cast. The row path can simply pass `NATIVE_TIMEZONE`; no
timestamp values are converted.
--
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]
