You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "rluvaton (via GitHub)" <gi...@apache.org> on 2026/06/15 14:01:01 UTC
[PR] refactor(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
rluvaton opened a new pull request, #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657
> [!NOTE]
> This does not remove all the shims in favor of the new infra as it will be very large PR
> instead it moves some stuff to show how it is being used and the process of doing so
A small macro-annotation library that lets a **single source file** carry code for multiple
Spark versions. You annotate a member (`def` / `class` / `object`) with a version range, and at *
*compile time** the macro keeps it, drops it, or tweaks it based on the version the build is
targeting.
## The problem it solves
Version-specific code used to live in parallel source folders (`src/main/spark-3.4/…`,
`spark-3.5/…`, `spark-4.0/…`) - so a whole file got copied per version just because one method
differed, or because a type like `WindowGroupLimitExec` only exists in Spark 3.5+. `@enableIfVer`
lets
those differences live **inline, in one file**: no duplication, and the version logic sits right
next to the code it gates.
## How you use it
The targeted versions are supplied by the build per "dimension" (`spark`). You write
a [semver](https://github.com/semver4j/semver4j#-ranges) range
```scala
import org.apache.comet.enableIfVer
// keep this method only on Spark 3.5+
@enableIfVer(spark = ">=3.5.0")
def onlyOn35Plus(): Unit =
...
// two same-named methods, one per version range - exactly one survives per build
@enableIfVer(spark = "<3.5.0") def isWGL(p: SparkPlan): Boolean = false
@enableIfVer(spark = ">=3.5.0") def isWGL(p: SparkPlan): Boolean = p.isInstanceOf[WindowGroupLimitExec]
```
### The four behaviors
| Annotation | On a non-matching version |
|------------------------|---------------------------------------------------------------------------------------------------------------------------------------------|
| `@enableIfVer` | **drops** the member entirely |
| `@implementIfVer` | **keeps the class/object** but empties its body and remove the inheritance (so a type that references 3.5-only symbols still exists on 3.4) |
| `@enableOverrideIfVer` | **strips `override`** (for a member that only overrides on some versions) |
### Ranges
Matched by semver4j: full `major.minor.patch` versions with `>` `>=` `<` `<=` `=` `!=`,
space = AND, `||` = OR, `A - B` hyphen ranges, x-ranges / partials and more (FYI, it supports `package.json` format)
## What it gives us
- **No more per-version source-folder duplication** for small differences.
- **One file, version logic inline** - easier to read and maintain.
- **Compile-time safety**: non-matching code (and its references to types that don't exist in that
version) is removed before type-checking, so each build only ever sees valid code.
- **Extensible & general**: any number of dimensions, combined with AND/OR, plus an explicit
"not enabled" state - all from one unified `@enableIfVer` namespace.
## Caveats
- `@enableIfVer` annotates **definitions only** - not statements (e.g. a bare `test(...)` call). To
gate a statement, wrap it in a `def` and annotate that.
- It cannot be applied to a **top-level class that has a companion object** (a macro-paradise
limitation). For whole top-level classes that are entirely version-specific, the version-specific
source folder is still the right tool.
## Apache Auron (Blaze)
most of the annotation code taken from Blaze/auron in [sparkver.scala]
(https://github.com/apache/auron/blob/a0e2b5f787073c50d24bbce9e7059f2e22e1dbf1/spark-version-annotation-macros/src/main/scala/org/apache/auron/sparkver.scala)
but updated the syntax of matching versions and replace `@sparkverEnableMembers` with
`@implementIfVer` that remove the inheritance as well - useful for abstract class
Blaze uses `/` to split between supported versions
so for example
like [this](https://github.com/apache/auron/blob/a0e2b5f787073c50d24bbce9e7059f2e22e1dbf1/spark-extension/src/main/scala/org/apache/spark/sql/hive/execution/auron/plan/NativeHiveTableScanBase.scala#L146-L148):
```scala
@sparkver("3.0 / 3.1 / 3.2 / 3.3 / 3.4 / 3.5 / 4.0 / 4.1")
override def simpleString(maxFields: Int): String =
s"$nodeName (${basedHiveScan.simpleString(maxFields)})"
```
This however use semver match syntax that is more widely known
so the same code for us will be:
```scala
@enableIfVer(spark = ">=3.0.0 <4.2.0")
override def simpleString(maxFields: Int): String =
s"$nodeName (${basedHiveScan.simpleString(maxFields)})"
```
and if you want to automatically have this function for every spark version above 3.0 and not only
until 4.1, you can do:
```scala
@enableIfVer(spark = ">=3.0.0")
override def simpleString(maxFields: Int): String =
s"$nodeName (${basedHiveScan.simpleString(maxFields)})"
```
the syntax is matching the one used in [`package.json`](https://docs.npmjs.com/cli/v11/configuring-npm/package-json#dependencies)
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4775698040
> One more question: does this approach only work for scala?
Yes
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4722232245
Thanks @rluvaton. I will take a look at this soon, but I am concerned that this is more complex than the current approach. How well does this approach work with IDEs, such as IntelliJ?
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton closed pull request #4657: refactor(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication
URL: https://github.com/apache/datafusion-comet/pull/4657
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4775700901
I'm closing this pr as it seems that there is not much interest
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4719410476
@andygrove can you please review? I think this will improve the scala code quality greatly
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4773877605
I created a PR to add a `spark-4.1+` shim folder to avoid the current duplication between 4.1 and 4.2 shims. This approach could be extended further if we want to be more aggressive with avoiding duplication.
https://github.com/apache/datafusion-comet/pull/4710
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4728702032
> Thanks @rluvaton. I will take a look at this soon, but I am concerned that this is more complex than the current approach.
I think this will:
## Reduce line count
you don't need to duplicate shims code when they are the same
## Make it easier to read
When you look at specific shim directory like `spark 3.5` you should also pay attention to the shim directory `spark 3.x` instead of just looking at the shim code and understand right away
## Adding shims from specific spark version and up
for example, `HashPartitioningLike` was added in spark 4 and backported to spark `3.5` and spark `3.4` and released in spark `3.5.1`
in the current approach, how would you create a function that check if spark partitioning is hash based if the `HashPartitioningLike` was **not** backported to spark 3.4 **which you still have shims for**?
with this pr you could write:
```scala
@enableIfVer(spark = ">= 3.5.1")
def isHashPartitioningLike(partitioning: Partitioning): Boolean = {
import org.apache.spark.sql.catalyst.plans.physical.HashPartitioningLike
partitioning match {
case hashLike: HashPartitioningLike => true
case par => false
}
}
@enableIfVer(spark = "< 3.5.1")
def isHashPartitioningLike(partitioning: Partitioning): Boolean = {
import org.apache.spark.sql.catalyst.plans.physical.HashPartitioning
partitioning match {
case hash: HashPartitioning => true
case par => false
}
}
```
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4728724604
> +0 for this, I prefer the separated source directories approach; introducing potentially incompatible classes or methods into a single source file seems more difficult to maintain.
you can use wildcard imports to get around it in most cases or scope the imports to the used 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] refactor(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4742637290
Another concern I have is how this impacts downstream forks that integrate with proprietary Spark versions (cloud vendors, for example). It seems like this approach will result in a lot of merge conflicts compared to having separate source folders.
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4725359429
+0 for this, I prefer the separated source directories approach; introducing potentially incompatible classes or methods into a single source file seems more difficult to maintain.
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "wForget (via GitHub)" <gi...@apache.org>.
wForget commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4737018353
One more question: does this approach only work for scala?
--
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(shim): add new shim infra with `enableIfVer` macro to avoid shims duplication [datafusion-comet]
Posted by "rluvaton (via GitHub)" <gi...@apache.org>.
rluvaton commented on PR #4657:
URL: https://github.com/apache/datafusion-comet/pull/4657#issuecomment-4720712320
Maybe @mbutrovich can look at it as well?
--
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