You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "schenksj (via GitHub)" <gi...@apache.org> on 2026/05/30 03:10:26 UTC
[PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
schenksj opened a new pull request, #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532
## Which issue does this PR close?
Closes #4527.
## Rationale for this change
Spark wraps file-source partition columns and other per-batch constants in `ConstantColumnVector`. When such a batch reaches Comet's serialization path (`Utils.getBatchFieldVectors`, used by broadcast/shuffle) or the FFI export path (`NativeUtil.exportBatch`), it was rejected with:
```
Comet execution only takes Arrow Arrays, but got ...ConstantColumnVector
```
This is a standalone fix; it was surfaced while working on the Delta Lake contrib integration (the OPTIMIZE / deletion-vector rewrite paths pull constants through a Comet operator), so prioritizing it helps that effort, but it applies to any plan that routes a constant column through a Comet operator.
## What changes are included in this PR?
- `ConstantColumnVectors.materialize` (in the `org.apache.spark.sql.comet.execution.arrow` package) builds a fresh Arrow `FieldVector` holding the constant repeated `numRows` times. It reuses the existing per-type `ArrowFieldWriter`s, so it covers every type -- scalars, decimal, timestamps, and complex struct/array/map -- and stays in sync with Spark's type handling, rather than a hand-rolled per-type switch.
- `Utils.materializeConstantColumnVector` exposes it to the serialization path.
- New match arms in `Utils.getBatchFieldVectors` and `NativeUtil.exportBatch` materialize a `ConstantColumnVector` instead of throwing. The existing `CometVector` path is untouched.
## How are these changes tested?
New test in `UtilsSuite` round-trips a batch with a value `ConstantColumnVector` and a null `ConstantColumnVector` through `serializeBatches` / `decodeBatches` and asserts the materialized values (and nulls) survive. The test fails on `main` with the "only takes Arrow Arrays" exception and passes with this change. `UtilsSuite` (3/3) and `CometExecSuite` (126/0) pass. The FFI `exportBatch` arm shares the same `materializeConstantColumnVector` helper.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-5096348167
> Is this ready to merge? @schenksj @mbutrovich @andygrove
It's been a few weeks, and though it doesn't have merge conflicts, we should make sure it still passes everything on `main`. I'll upmerge it now and we can consider it "approved pending CI."
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "schenksj (via GitHub)" <gi...@apache.org>.
schenksj commented on code in PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#discussion_r3488519298
##########
spark/src/main/scala/org/apache/spark/sql/comet/util/Utils.scala:
##########
@@ -426,4 +440,39 @@ object Utils extends CometTypeShim with Logging {
throw new SparkException(s"Unsupported Arrow Vector for $reason: ${valueVector.getClass}")
}
}
+
+ /**
+ * Materialize a Spark `ConstantColumnVector` into a fresh Arrow `FieldVector` whose value is
+ * the same constant repeated `numRows` times.
+ *
+ * Spark wraps file-source partition columns and other per-batch constants in
+ * `ConstantColumnVector`; downstream Comet operators feeding `NativeUtil.exportBatch` or
+ * `getBatchFieldVectors` trip on it because those paths only handle `CometVector`. This helper
+ * materializes the constant into an Arrow vector inline.
+ *
+ * The caller owns the returned vector and must close it (or hand it to Arrow's exporter, which
+ * transfers ownership). The vector is allocated against `allocator`, sized to exactly
+ * `numRows`, and pre-filled with the constant value (or null when `cv.isNullAt(0)`).
+ *
+ * All Spark types are supported (delegates to the per-type ArrowFieldWriters, which include
+ * struct/array/map); throws only for a type Arrow itself can't represent.
+ */
+ def materializeConstantColumnVector(
+ cv: ConstantColumnVector,
+ dt: DataType,
+ numRows: Int,
+ name: String,
+ allocator: BufferAllocator): FieldVector = {
+ // "UTC" is deliberate here, NOT the session-local timezone that `toArrowSchema` threads
+ // through. These constants are materialised alongside non-constant columns in the same
+ // batch/`VectorSchemaRoot`, and Comet's non-constant `TimestampType` columns are Arrow
+ // vectors exported from native execution, where Comet always tags them `Timestamp(us, "UTC")`
+ // (see native `serde.rs`). Spark itself stores `TimestampType` as micros in UTC, so the
+ // constant's value is already a UTC instant. Tagging the materialised constant "UTC" keeps its
+ // Arrow timezone metadata consistent with its sibling timestamp columns; threading the
+ // session-local timezone here would instead introduce the mismatch. `TimestampNTZType` carries
+ // no zone regardless of this argument.
+ org.apache.spark.sql.comet.execution.arrow.ConstantColumnVectors
Review Comment:
Done in 18c147f19 — went with having both callers (`getBatchFieldVectors` and `NativeUtil.exportBatch`) call `ConstantColumnVectors.materialize` directly and deleted the `Utils.materializeConstantColumnVector` forwarder. The `"UTC"` rationale now lives once, on `ConstantColumnVectors`, with a one-line pointer at each call site. (Also dropped the now-unused `BufferAllocator` import from `Utils.scala`.)
##########
spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala:
##########
@@ -137,6 +137,19 @@ class NativeUtil {
provider,
arrowArray,
arrowSchema)
+ case cv: org.apache.spark.sql.execution.vectorized.ConstantColumnVector =>
Review Comment:
Done in 18c147f19 — imported `ConstantColumnVector` (and `ConstantColumnVectors`) and use the short names, matching the rest of the file.
##########
spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala:
##########
@@ -137,6 +137,19 @@ class NativeUtil {
provider,
arrowArray,
arrowSchema)
+ case cv: org.apache.spark.sql.execution.vectorized.ConstantColumnVector =>
+ // Spark uses ConstantColumnVector for partition columns / per-batch constants (e.g.
+ // partition values, synthetic columns). Materialise to a fresh Arrow vector so Comet's
+ // native side -- which expects Arrow Arrays only -- can ingest the batch. Without this,
+ // queries that pull constants through a Comet operator fail with "Comet execution only
+ // takes Arrow Arrays".
+ val rows = batch.numRows()
Review Comment:
Done in 18c147f19 — the arm now reads `numRows += materialised.getValueCount` after the writer finishes, symmetric with the `CometVector` arm.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "schenksj (via GitHub)" <gi...@apache.org>.
schenksj commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-4827359543
Good question. Short version: for a normal Comet-native scan of a partitioned table this code never runs — partition values aren't `ConstantColumnVector` on that path. Comet's native scan serializes partition values into the native plan and DataFusion expands them into Arrow columns natively (`CometNativeScan.scala:169-191`, `partition2Proto`), so the 1M-batch partitioned-scan case you describe incurs zero cost from this change.
**Why Delta hits this at all (the motivating case):** contrib-delta accelerates Delta reads natively — partition values, column-mapping, and row-tracking are all synthesized *inside* the kernel/native scan, so its output is pure Arrow and never contains a `ConstantColumnVector`. But certain Delta *write* rewrites — OPTIMIZE bin-packing and the deletion-vector DELETE/UPDATE/MERGE rewrites — need read semantics the native scan can't faithfully reproduce. For example, with `deletionVectors.useMetadataRowIndex=false` the rewrite requests two distinctly-named row-index columns and native can only synthesize one; the inverted row-index-filter semantics and the column-mapping-materialized `row_commit_version` similarly can't be reconstructed from kernel enumeration. For exactly those reads the contrib *deliberately declines* native acceleration and hands the scan back to vanilla Spark's `FileSourceScanExec` — that's the "Comet → plain" step, and it's a correctness decision, not
an oversight. Spark's vectorized reader then emits the partition and Delta-synthetic columns (`__delta_internal_is_row_deleted`, the row-index, row-tracking ids) as `ConstantColumnVector`s.
The rest of the rewrite plan *above* that one declined scan is still Comet-accelerated — the repartition/bin-pack and the columnar-to-row step that feeds Delta's writer — so the constant-bearing Spark batch crosses *back* into a Comet operator (the "plain → Comet" step). That's where it reaches `getBatchFieldVectors` (shuffle/broadcast serialize) or `NativeUtil.exportBatch` (FFI), which previously only handled `CometVector` and threw "Comet execution only takes Arrow Arrays". So the round trip isn't Comet leaking constants out of native execution — it's a single deliberately-declined scan sandwiched in an otherwise-native plan, with Comet operators on both sides of it.
So this materialization is per-batch, O(numRows) per constant column, but only in that bridging window. I looked at whether a cheaper encoding could avoid the expansion there, and it can't without new native work: the FFI export side requires a real Arrow `FieldVector`, and the native import side normalizes everything to materialized arrays — dictionaries are proactively unpacked (`scan.rs:99-100`) and RunEndEncoded isn't supported at all, with no scalar-backed column type crossing FFI. A dictionary would just get unpacked on entry (`scan.rs:161`), erasing the saving. And on `main` this case isn't cheaper — it throws `SparkException`. So it's correctness vs. crash on a non-hot path, not a per-batch regression on scans. Happy to file a follow-up if we ever want a true constant/REE column across FFI, but that's a sizable native feature.
(Disclosure: this analysis and reply were written with help from AI — Claude 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-4802231705
Thanks for addressing feedback @schenksj. Could you add the new suite to the `pr_build*.yml` workflows to fix the CI failure:
```
Suite not found in workflow .github/workflows/pr_build_linux.yml: org.apache.comet.vector.NativeUtilSuite
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "schenksj (via GitHub)" <gi...@apache.org>.
schenksj commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-4753349260
Thanks for the thorough review, @andygrove! All four points are addressed. (Disclosure: this PR's changes and this response were written with help from AI — Claude Code.)
**1. Hardcoded `"UTC"` timezone.** I went with the comment route, because on investigation `"UTC"` turns out to be the *correct* choice here rather than just a defensible one — and threading the session-local timezone would actively introduce the mismatch you were worried about. These constants are materialized alongside the batch's non-constant columns in the same `VectorSchemaRoot`, and Comet's non-constant `TimestampType` columns are Arrow vectors exported from native execution, where Comet always tags them `Timestamp(µs, "UTC")` (see native `serde.rs`). Spark itself stores `TimestampType` as micros in UTC, so the constant's value is already a UTC instant. Tagging the materialized constant `"UTC"` keeps its Arrow timezone metadata consistent with its sibling timestamp columns; threading `SQLConf.sessionLocalTimeZone` would make them diverge. I've expanded the comment in `materializeConstantColumnVector` to spell this out.
**2. Test coverage in `UtilsSuite`.** Added two cases beyond `IntegerType`:
- A nullable `StructType` constant (with a null nested field and a wholly-null struct) — exercises the `getStruct(rowId)` / `getChild(ordinal)` `ArrowFieldWriter` path.
- A `TimestampType` constant — round-trips the micros and pins down the `"UTC"` choice, guarding against anyone later swapping the zone argument.
**3. FFI `exportBatch` arm.** Done — added `NativeUtilSuite`, which runs a `ConstantColumnVector`-containing batch (scalar value, null, and a struct constant) through the actual Arrow C Data Interface via `NativeUtil.exportBatchToAddresses` + `importVector` — the same export/import round trip `getNextBatch` performs in production, so it covers `Data.exportVector` + the allocator handoff. I verified it's a real guard: removing the `ConstantColumnVector` arm in `exportBatch` makes it fail with `Comet execution only takes Arrow Arrays`.
**4. CI flake.** Agreed — the `Spark 4.1 / sql_core-3` failure was the runner-setup infra flake. I've also merged latest `main` into the branch (it was out of date with base), so a fresh CI run picks all of this up.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "kazuyukitanimura (via GitHub)" <gi...@apache.org>.
kazuyukitanimura commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-5096319080
Is this ready to merge? @schenksj @mbutrovich @andygrove
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-4742461444
A few observations from a review pass. Acknowledging up front that this comment was drafted with help from AI (Claude Code).
The reuse of the existing per-type `ArrowFieldWriter`s is exactly the right design choice — every Spark type comes along for free and the logic stays in sync with non-constant batches.
A few notes:
1. **Hardcoded `"UTC"` timezone in `Utils.materializeConstantColumnVector`.** The choice is defensible since Spark stores `TimestampType` as micros in UTC, but elsewhere in this file `toArrowSchema` requires callers to pass the session timezone explicitly. If a batch flows through `getBatchFieldVectors` containing both a `ConstantColumnVector` of `TimestampType` and a non-constant timestamp column, would they end up with mismatched Arrow timezone metadata? Worth either threading a `timeZoneId` parameter through `getBatchFieldVectors` (sourced from `SQLConf.sessionLocalTimeZone`), or adding a comment explaining why `"UTC"` is safe in this serialization context.
2. **Test coverage in `UtilsSuite`.** Nice round-trip test. Would you mind adding a case or two beyond `IntegerType`? A nullable `StructType` or `ArrayType` constant exercises a different `ArrowFieldWriter` code path (via `getStruct(rowId)` / `getArray(rowId)` on `ConstantColumnVector`), and a `TimestampType` constant would pin down the timezone choice in case anyone later changes it. The materializer claims to support every type and it'd be useful to prove it for at least the complex case.
3. **FFI `exportBatch` arm.** It shares `materializeConstantColumnVector` with `getBatchFieldVectors`, so the materializer itself is covered. The surrounding code (`Data.exportVector` + the allocator handoff) isn't exercised by the new test. If easy, a small smoke test that runs a `ConstantColumnVector`-containing batch through an actual Comet operator path would catch a regression in the FFI wiring. Optional.
4. **CI flake.** The one failing check (`Spark 4.1 sql_core-3`) annotates as `The hosted runner lost communication with the server` — infrastructure, not the PR. The matching `sql_core-3` job on Spark 3.5 passes. A re-run should clear it.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "schenksj (via GitHub)" <gi...@apache.org>.
schenksj commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-4698705914
Gentle ping — still looking for a first review on this one. Small, self-contained fix. Thanks!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-4802235816
@mbutrovich @comphead could you also 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#discussion_r3477818155
##########
spark/src/main/scala/org/apache/spark/sql/comet/util/Utils.scala:
##########
@@ -426,4 +440,39 @@ object Utils extends CometTypeShim with Logging {
throw new SparkException(s"Unsupported Arrow Vector for $reason: ${valueVector.getClass}")
}
}
+
+ /**
+ * Materialize a Spark `ConstantColumnVector` into a fresh Arrow `FieldVector` whose value is
+ * the same constant repeated `numRows` times.
+ *
+ * Spark wraps file-source partition columns and other per-batch constants in
+ * `ConstantColumnVector`; downstream Comet operators feeding `NativeUtil.exportBatch` or
+ * `getBatchFieldVectors` trip on it because those paths only handle `CometVector`. This helper
+ * materializes the constant into an Arrow vector inline.
+ *
+ * The caller owns the returned vector and must close it (or hand it to Arrow's exporter, which
+ * transfers ownership). The vector is allocated against `allocator`, sized to exactly
+ * `numRows`, and pre-filled with the constant value (or null when `cv.isNullAt(0)`).
+ *
+ * All Spark types are supported (delegates to the per-type ArrowFieldWriters, which include
+ * struct/array/map); throws only for a type Arrow itself can't represent.
+ */
+ def materializeConstantColumnVector(
+ cv: ConstantColumnVector,
+ dt: DataType,
+ numRows: Int,
+ name: String,
+ allocator: BufferAllocator): FieldVector = {
+ // "UTC" is deliberate here, NOT the session-local timezone that `toArrowSchema` threads
+ // through. These constants are materialised alongside non-constant columns in the same
+ // batch/`VectorSchemaRoot`, and Comet's non-constant `TimestampType` columns are Arrow
+ // vectors exported from native execution, where Comet always tags them `Timestamp(us, "UTC")`
+ // (see native `serde.rs`). Spark itself stores `TimestampType` as micros in UTC, so the
+ // constant's value is already a UTC instant. Tagging the materialised constant "UTC" keeps its
+ // Arrow timezone metadata consistent with its sibling timestamp columns; threading the
+ // session-local timezone here would instead introduce the mismatch. `TimestampNTZType` carries
+ // no zone regardless of this argument.
+ org.apache.spark.sql.comet.execution.arrow.ConstantColumnVectors
Review Comment:
Small one: `Utils.materializeConstantColumnVector` is a one-line forward to `ConstantColumnVectors.materialize` with `"UTC"` hardcoded. Could the two callers (`getBatchFieldVectors` and `NativeUtil.exportBatch`) call `ConstantColumnVectors.materialize` directly and let the UTC rationale live once on that method? The current shape duplicates the rationale doc across both files. Alternative: drop the `timeZoneId` parameter from `ConstantColumnVectors.materialize` entirely and bake `"UTC"` in there, since the comment justifies UTC unconditionally.
##########
spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala:
##########
@@ -137,6 +137,19 @@ class NativeUtil {
provider,
arrowArray,
arrowSchema)
+ case cv: org.apache.spark.sql.execution.vectorized.ConstantColumnVector =>
Review Comment:
Could `ConstantColumnVector` be imported and `Utils.materializeConstantColumnVector` use the existing `Utils` import on line 28? Matches the rest of the file.
##########
spark/src/main/scala/org/apache/comet/vector/NativeUtil.scala:
##########
@@ -137,6 +137,19 @@ class NativeUtil {
provider,
arrowArray,
arrowSchema)
+ case cv: org.apache.spark.sql.execution.vectorized.ConstantColumnVector =>
+ // Spark uses ConstantColumnVector for partition columns / per-batch constants (e.g.
+ // partition values, synthetic columns). Materialise to a fresh Arrow vector so Comet's
+ // native side -- which expects Arrow Arrays only -- can ingest the batch. Without this,
+ // queries that pull constants through a Comet operator fail with "Comet execution only
+ // takes Arrow Arrays".
+ val rows = batch.numRows()
Review Comment:
Minor: the existing `CometVector` arm reads `valueVector.getValueCount` for `numRows`. This arm reads `batch.numRows()` directly. Reading `materialised.getValueCount` after the writer finishes would be symmetric with the other arm and catch a hypothetical writer underrun.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] fix: materialize ConstantColumnVector on Comet's serialize/export paths [datafusion-comet]
Posted by "schenksj (via GitHub)" <gi...@apache.org>.
schenksj commented on PR #4532:
URL: https://github.com/apache/datafusion-comet/pull/4532#issuecomment-4827360772
Thanks @mbutrovich, @andygrove — all review feedback is addressed in 18c147f19:
- @andygrove's Preflight failure: registered `org.apache.comet.vector.NativeUtilSuite` in both `pr_build_linux.yml` and `pr_build_macos.yml` (next to `UtilsSuite`).
- @mbutrovich's three nits (replied inline): dropped the `Utils.materializeConstantColumnVector` forwarder so both callers hit `ConstantColumnVectors.materialize` directly with the UTC rationale living once on that method; switched `NativeUtil` to the imported short names; and made the export arm read `numRows += materialised.getValueCount`, symmetric with the `CometVector` arm.
Verified locally: `NativeUtilSuite` + `UtilsSuite` pass (6/6) on Spark 4.1/2.13, and the spark module also compiles clean under Scala 2.12 (Spark 3.4).
(Disclosure: these changes and this response were written with help from AI — Claude 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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org