Skip to content

[SPARK-49828][SQL] Make Column(expression) usable outside of the org.apache.spark package - #57759

Open
peter-toth wants to merge 2 commits into
apache:masterfrom
peter-toth:SPARK-49828-expr-to-column
Open

[SPARK-49828][SQL] Make Column(expression) usable outside of the org.apache.spark package#57759
peter-toth wants to merge 2 commits into
apache:masterfrom
peter-toth:SPARK-49828-expr-to-column

Conversation

@peter-toth

Copy link
Copy Markdown
Contributor

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:

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:

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:

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

…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

uros-b commented Aug 4, 2026

Copy link
Copy Markdown
Member

LGTM

sql/api is a scalafmt governed module with maxColumn = 98, one line was 99.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants