You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "sam-1112 (via GitHub)" <gi...@apache.org> on 2026/09/23 08:55:53 UTC
[PR] refactor: remove unused cast compatibility flag [datafusion-comet]
sam-1112 opened a new pull request, #6135:
URL: https://github.com/apache/datafusion-comet/pull/6135
## Which issue does this PR close?
Closes #5077.
## Rationale for this change
`allowIncompatible` controls whether the JVM planner may offload casts that are not guaranteed to be fully compatible. It is a JVM planning decision, but it was unnecessarily serialized in the `Cast` protobuf and carried through native cast options despite having no native consumer.
Keeping this unused flag in the JVM-to-native contract obscures the ownership of the compatibility decision and leaves dead state in the native cast API.
## What changes are included in this PR?
- Stop serializing `allow_incompat` in `Cast` expressions.
- Reserve the removed protobuf field number and name to preserve wire compatibility.
- Remove the unused field and constructor parameters from `SparkCastOptions`.
- Update native call sites, including benchmarks, for the simplified API.
- Remove unused JVM-side helper methods.
- Correct the cast expression audit to state that `allowIncompatible` is enforced by the JVM planner only.
## How are these changes tested?
No new tests were added because this PR removes unused JVM-to-native cast-option plumbing and does not change cast behavior.
--
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] refactor: remove unused cast compatibility flag [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6135:
URL: https://github.com/apache/datafusion-comet/pull/6135#discussion_r4083355095
##########
spark/src/main/scala/org/apache/comet/expressions/CometCast.scala:
##########
@@ -155,10 +155,6 @@ object CometCast
castBuilder.setChild(childExpr)
castBuilder.setDatatype(dataType)
castBuilder.setEvalMode(evalModeToProto(evalMode))
- castBuilder.setAllowIncompat(
- SQLConf.get
- .getConfString(CometConf.getExprAllowIncompatConfigKey(classOf[Cast]), "false")
- .toBoolean)
Review Comment:
This removes the last use of `CometConf` in this file, so `import org.apache.comet.CometConf` at the top is now unused. The `lint-java` jobs run scalafix's `RemoveUnused` rule in check mode, so this will fail all four of them. Could you remove the import? `make format` handles it too.
--
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] refactor: remove unused cast compatibility flag [datafusion-comet]
Posted by "sam-1112 (via GitHub)" <gi...@apache.org>.
sam-1112 commented on PR #6135:
URL: https://github.com/apache/datafusion-comet/pull/6135#issuecomment-6000432704
@andygrove It's ready for another review. Sorry for the late heads-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] refactor: remove unused cast compatibility flag [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove merged PR #6135:
URL: https://github.com/apache/datafusion-comet/pull/6135
--
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] refactor: remove unused cast compatibility flag [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6135:
URL: https://github.com/apache/datafusion-comet/pull/6135#issuecomment-5995653304
@sam-1112 let me know when this is ready for another 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] refactor: remove unused cast compatibility flag [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #6135:
URL: https://github.com/apache/datafusion-comet/pull/6135#discussion_r4156049996
##########
spark/src/main/scala/org/apache/comet/rules/RevertNativeForTransitionHeavyStages.scala:
##########
@@ -198,7 +198,28 @@ case class RevertNativeForTransitionHeavyStages(session: SparkSession, wholePlan
case local: CometLocalTopKExec => local.child
case cometExec: CometExec =>
if (cometExec.originalPlan.children.size == cometExec.children.size) {
- cometExec.originalPlan.withNewChildren(cometExec.children)
+ val originalWithCurrentExpressions = cometExec match {
Review Comment:
This change and its two tests are not part of the `allowIncompatible` cleanup. The `CometNativeScanExec` branch and the `RevertNativeForTransitionHeavyStagesSuite` test are the same fix as #6137 (which closes #5486), so whichever PR lands second will conflict on this hunk. The `CometIcebergNativeScanExec` branch is also only in this PR, so it will not be reviewed or merged with the V1 fix. The description here still says the PR does not change behavior and adds no tests. Could we drop `cefe845a1` from this PR and move the Iceberg case into #6137? That keeps this PR a pure no-behavior-change cleanup and lets the DPP fix be reviewed with its own description.
--
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] refactor: remove unused cast compatibility flag [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #6135:
URL: https://github.com/apache/datafusion-comet/pull/6135#issuecomment-5876181206
This is a light fully automated review since there are so many PRs open.
The `reserved 5` / `reserved "allow_incompat"` lines in `expr.proto` match how retired fields are handled elsewhere. But the Rust test targets no longer compile at the current head. `SparkParquetOptions::new` (`native/core/src/parquet/parquet_support.rs:126`) and `SparkCastOptions::new` (`native/spark-expr/src/conversion_funcs/cast.rs:147`) now take two arguments, and I count 14 call sites in `#[cfg(test)]` modules across 6 files that still pass three. Some examples are `native/core/src/parquet/schema_adapter.rs:3106`, `native/core/src/parquet/parquet_support.rs:2130`, `native/core/src/parquet/cast_column.rs:396`, `native/core/src/execution/operators/iceberg_scan.rs:633`, and `native/spark-expr/src/conversion_funcs/cast.rs:1249`. That is the same `E0061` shape that was flagged once on `schema_adapter.rs` earlier in review. Most of them are the `issue_5783_*` duplicate field id tests from #5654, which came in with the latest merge of `main`. So `cargo test` and `cargo clippy --all-
targets` fail even though the library itself builds. Could you update these calls to the two-argument form?
--
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