Copilot commented on code in PR #13018:
URL: https://github.com/apache/gluten/pull/13018#discussion_r4034713676
##########
gluten-arrow/src/main/java/org/apache/gluten/columnarbatch/ColumnarBatches.java:
##########
@@ -196,7 +197,10 @@ public static ColumnarBatch load(BufferAllocator
allocator, ColumnarBatch input)
ArrowSchema arrowSchema = ArrowSchema.allocateNew(allocator);
CDataDictionaryProvider provider = new CDataDictionaryProvider()) {
ColumnarBatchJniWrapper.exportToArrow(
- iv.handle(), cSchema.memoryAddress(), cArray.memoryAddress());
+ iv.handle(),
+ cSchema.memoryAddress(),
+ cArray.memoryAddress(),
+ SparkSchemaUtil.enableLargeVarTypes());
Review Comment:
This flag is now passed for every backend, but not every `ColumnarBatch`
implementation honors it: `BoltColumnarBatch`, `ArrowColumnarBatch`, and
`ArrowCStructColumnarBatch` still export 32-bit-offset arrays. With the conf
enabled, `ArrowUtil.toArrowSchema` nevertheless rebuilds their schema as
`LargeUtf8`/`LargeBinary`, so the imported C array and schema disagree and can
produce invalid values or import failures. Please either implement large-layout
conversion for these implementations or gate this argument to backends that
actually export large variable-width buffers.
##########
cpp/bolt/memory/BoltColumnarBatch.cc:
##########
@@ -64,14 +64,14 @@ void BoltColumnarBatch::ensureFlattened() {
flattened_ = true;
}
-std::shared_ptr<ArrowSchema> BoltColumnarBatch::exportArrowSchema() {
+std::shared_ptr<ArrowSchema> BoltColumnarBatch::exportArrowSchema(bool) {
auto out = std::make_shared<ArrowSchema>();
ensureFlattened();
bolt::exportToArrow(rowVector_, *out, ArrowUtils::getBridgeOptions());
return out;
}
-std::shared_ptr<ArrowArray> BoltColumnarBatch::exportArrowArray() {
+std::shared_ptr<ArrowArray> BoltColumnarBatch::exportArrowArray(bool) {
auto out = std::make_shared<ArrowArray>();
ensureFlattened();
bolt::exportToArrow(rowVector_, *out, rowVector_->pool(),
ArrowUtils::getBridgeOptions());
Review Comment:
This override discards the new flag and always exports 32-bit-offset Arrow
variable-width arrays. `ColumnarBatches.load` passes the SQL conf through and
reconstructs a LargeUtf8/LargeBinary schema when it is enabled, so using the
flag with Bolt makes the Java importer interpret this regular array with the
wrong layout. Pass the flag through a Bolt bridge option if supported, or keep
the Bolt import schema on regular variable-width types instead of advertising
large-type support.
##########
gluten-arrow/src/main/scala/org/apache/spark/sql/utils/SparkArrowUtil.scala:
##########
@@ -162,9 +179,12 @@ object SparkArrowUtil {
}
/** Maps schema from Spark to Arrow. NOTE: timeZoneId required for
TimestampType in StructType */
- def toArrowSchema(schema: StructType, timeZoneId: String): Schema = {
+ private[utils] def toArrowSchema(
+ schema: StructType,
+ timeZoneId: String,
+ largeVarTypes: Boolean = false): Schema = {
Review Comment:
Making `toArrowSchema` package-private to `org.apache.spark.sql.utils`
breaks the existing Bolt callers, which are in `org.apache.spark.sql.execution`
and still invoke `SparkArrowUtil.toArrowSchema` (for example
`backends-bolt/src/main/scala/org/apache/spark/sql/execution/ColumnarBuildSideRelation.scala:100`).
The Bolt module will fail to compile against this change. Keep this API
accessible or migrate every remaining caller before restricting its visibility.
--
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]
