peter-toth opened a new pull request, #57759:
URL: https://github.com/apache/spark/pull/57759
### What changes were proposed in this pull request?
This makes the intended `Column(expression)` conversion usable from outside
the `org.apache.spark` package:
1. `object Column` (`sql/api`) becomes public. Every member of it is already
`private[spark]` or narrower, so no new member becomes visible -- the object
simply becomes nameable, which is all the extension in point 2 needs. The two
`apply` overloads that relied on the object's own visibility are now marked
`private[spark]` explicitly.
2. A named `@DeveloperApi ClassicConversions.column(e: Expression): Column`
is added, for callers who would rather not rely on an implicit.
3. A new `ExpressionToColumnSuite` in package `test.org.apache.spark.sql`,
i.e. outside `org.apache.spark`, pins both entry points and the round trip.
### Why are the changes needed?
`ClassicConversions.ColumnConstructorExt` is already a public
`@DeveloperApi`:
```scala
implicit class ColumnConstructorExt(val c: Column.type) {
def apply(e: Expression): Column = ExpressionUtils.column(e)
}
```
It is meant to let extension developers write `Column(expr)`, but it cannot
be used outside `org.apache.spark`, because `object Column` is itself
`private[spark]`. So `Column(expr)` does not even compile there:
```scala
package com.example.ext
// error: not found: value Column
val c = Column(Literal(1))
```
`ExpressionUtils`, `ExpressionColumnNode` and `ColumnNode` are package
private as well, and SPARK-49022 removed the public `new Column(expr:
Expression)` constructor, so there is currently no public Expression -> Column
path at all. The reverse direction is public and works fine
(`ColumnConversions.expression(col)`, `col.expr`), which makes the gap
asymmetric. Today the only workaround is to put the helper in a package under
`org.apache.spark`, which is what `mllib`'s `ml/stat/Summarizer.scala` does.
This also makes an existing review comment on #48306 true: @hvanhovell
suggested "You could call
`org.apache.spark.sql.classic.ClassicConversions.column` directly if you want
to avoid implicits" -- that method did not exist until now.
Prior art: #48306 by @holdenk took the broader approach of exposing the
`ColumnNode` AST types (`ExpressionColumnNode`,
`ColumnNodeToExpressionConverter`) and moving `columnNodeSupport.scala` out of
`sql.internal`. It was approved by @hvanhovell, then the approval was dismissed
over the package choice, and the stale bot closed it. Most of that diff is now
obsolete, since the file has meanwhile moved to `org.apache.spark.sql.classic`.
This PR intentionally stays narrower and leaves the AST types internal, so
Spark keeps freedom over their shape.
Three points reviewers may want to weigh in on:
- The named factory is deliberately on `object ClassicConversions` rather
than on the `ClassicConversions` trait. On the trait it shadows
`functions.column(colName: String)` for anyone who mixes the trait in, which
breaks existing call sites -- 6 of them in `sql/core`'s own tests, via
`QueryTest.testImplicits`. The trade-off is that a cross-version shim
implementing the trait does not inherit it.
- `import ClassicConversions._` combined with `import functions.column` is
now ambiguous. It is opt-in and a compile error rather than anything silent,
but happy to rename (`columnOf`, `toColumn`) if preferred; `column` was chosen
to mirror `ColumnConversions.expression`.
- `object Column` has no public members, so it shows up as an empty entry in
the generated API docs. I left it without an `@since` tag, since the object
itself has existed since 4.0, just not publicly.
### Does this PR introduce _any_ user-facing change?
Yes, it adds public API. No existing behavior changes.
- `object Column` becomes public (no new members).
- `ClassicConversions.column(e: Expression): Column` is new, tagged `@since
4.4.0`.
Extension developers outside `org.apache.spark` can now write either:
```scala
import org.apache.spark.sql.classic.ClassicConversions._
val c = Column(myExpression)
// or, without the implicit:
val c = ClassicConversions.column(myExpression)
```
### How was this patch tested?
New
`sql/core/src/test/scala/test/org/apache/spark/sql/ExpressionToColumnSuite.scala`.
Its package is outside `org.apache.spark`, so compiling it is as much of the
test as running it -- that is the piece #48306 lacked, which is how the
visibility gap went unnoticed in the first place.
- The suite fails to compile on unmodified `master` with `not found: value
Column` at both `Column(...)` sites, which is exactly the error a user hits.
- 3 tests pass with the change: both entry points, plus use in an actual
query.
- `ColumnExpressionSuite`, `JavaColumnExpressionSuite`, `DataFrameSuite`,
`DatasetSuite`, `JavaDatasetSuite` and the full `sql-api` test suite pass (~900
tests).
- MiMa is clean against `spark-sql_2.13:4.0.0` and
`spark-sql-api_2.13:4.0.0`, and `compile` is clean across all modules,
including `mllib`, which consumes the `Column(node)` overload.
### Was this patch authored or co-authored using generative AI tooling?
Generated-by: 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: [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]
