[SPARK-49828][SQL] Make Column(expression) usable outside of the org.apache.spark package - #57759
Open
peter-toth wants to merge 2 commits into
Open
[SPARK-49828][SQL] Make Column(expression) usable outside of the org.apache.spark package#57759peter-toth wants to merge 2 commits into
peter-toth wants to merge 2 commits into
Conversation
…apache.spark package `ClassicConversions.ColumnConstructorExt` is a public `@DeveloperApi` that adds a `Column(e: Expression)` factory to the `Column` companion, but it cannot be used outside of `org.apache.spark`, because `object Column` itself is `private[spark]`. `ExpressionUtils`, `ExpressionColumnNode` and `ColumnNode` are package private too, so since SPARK-49022 removed the public `new Column(expr: Expression)` constructor there has been no public Expression -> Column path at all. The reverse direction is public and works: `ColumnConversions.expression(col)`. This makes `object Column` public. Every member of it is already `private[spark]` or narrower, so the already shipped extension resolves while no new member becomes visible; the two `apply` overloads that relied on the object's own visibility are now marked `private[spark]` explicitly. It also adds a named `ClassicConversions.column(e)` for callers that prefer not to rely on an implicit. The named factory is on the object rather than on the `ClassicConversions` trait on purpose: on the trait it shadows `functions.column(colName: String)` for everyone who mixes the trait in, which breaks existing call sites. Prior art: apache#48306 by holdenk, which took the broader approach of exposing the ColumnNode AST types and was closed by the stale bot.
uros-b
approved these changes
Aug 4, 2026
Member
|
LGTM |
sql/api is a scalafmt governed module with maxColumn = 98, one line was 99.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
This makes the intended
Column(expression)conversion usable from outside theorg.apache.sparkpackage:object Column(sql/api) becomes public. Every member of it is alreadyprivate[spark]or narrower, so no new member becomes visible -- the object simply becomes nameable, which is all the extension in point 2 needs. The twoapplyoverloads that relied on the object's own visibility are now markedprivate[spark]explicitly.@DeveloperApi ClassicConversions.column(e: Expression): Columnis added, for callers who would rather not rely on an implicit.ExpressionToColumnSuitein packagetest.org.apache.spark.sql, i.e. outsideorg.apache.spark, pins both entry points and the round trip.Why are the changes needed?
ClassicConversions.ColumnConstructorExtis already a public@DeveloperApi:It is meant to let extension developers write
Column(expr), but it cannot be used outsideorg.apache.spark, becauseobject Columnis itselfprivate[spark]. SoColumn(expr)does not even compile there:ExpressionUtils,ExpressionColumnNodeandColumnNodeare package private as well, and SPARK-49022 removed the publicnew 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 underorg.apache.spark, which is whatmllib'sml/stat/Summarizer.scaladoes.This also makes an existing review comment on #48306 true: @hvanhovell suggested "You could call
org.apache.spark.sql.classic.ClassicConversions.columndirectly 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
ColumnNodeAST types (ExpressionColumnNode,ColumnNodeToExpressionConverter) and movingcolumnNodeSupport.scalaout ofsql.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 toorg.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:
object ClassicConversionsrather than on theClassicConversionstrait. On the trait it shadowsfunctions.column(colName: String)for anyone who mixes the trait in, which breaks existing call sites -- 6 of them insql/core's own tests, viaQueryTest.testImplicits. The trade-off is that a cross-version shim implementing the trait does not inherit it.import ClassicConversions._combined withimport functions.columnis now ambiguous. It is opt-in and a compile error rather than anything silent, but happy to rename (columnOf,toColumn) if preferred;columnwas chosen to mirrorColumnConversions.expression.object Columnhas no public members, so it shows up as an empty entry in the generated API docs. I left it without an@sincetag, 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 Columnbecomes public (no new members).ClassicConversions.column(e: Expression): Columnis new, tagged@since 4.4.0.Extension developers outside
org.apache.sparkcan now write either:How was this patch tested?
New
sql/core/src/test/scala/test/org/apache/spark/sql/ExpressionToColumnSuite.scala. Its package is outsideorg.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.masterwithnot found: value Columnat bothColumn(...)sites, which is exactly the error a user hits.ColumnExpressionSuite,JavaColumnExpressionSuite,DataFrameSuite,DatasetSuite,JavaDatasetSuiteand the fullsql-apitest suite pass (~900 tests).spark-sql_2.13:4.0.0andspark-sql-api_2.13:4.0.0, andcompileis clean across all modules, includingmllib, which consumes theColumn(node)overload.Was this patch authored or co-authored using generative AI tooling?
Generated-by: Claude Code