cloud-fan commented on code in PR #56164: URL: https://github.com/apache/spark/pull/56164#discussion_r3316905279
########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/GenerationExpression.java: ########## @@ -0,0 +1,55 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.spark.sql.connector.catalog; + +import org.apache.spark.annotation.Evolving; +import org.apache.spark.sql.connector.expressions.Expression; + +/** + * A class that represents generation expressions for computed/generated columns. + * <p> + * Connectors can define generation expressions using either a SQL string (Spark SQL dialect) or an + * {@link Expression expression} if the generation expression can be expressed as a supported + * connector expression. If both the SQL string and the expression are provided, Spark first + * attempts to convert the given expression to its internal representation. If the expression + * cannot be converted, and a SQL string is provided, Spark will fall back to parsing the SQL Review Comment: The Javadoc describes a Spark consumer behavior -- "Spark first attempts to convert the given expression to its internal representation. If the expression cannot be converted, and a SQL string is provided, Spark will fall back to parsing the SQL string." -- but no Spark code in this PR exercises that fallback. Production paths still read only the SQL via `Column.generationExpression()`. A reader would reasonably conclude Spark already prefers the V2 form; today it doesn't. Either point to the consumer (and explain when), or reword to make clear the V2 expression is provided **for connectors to consume**, not for Spark. ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Column.java: ########## @@ -78,19 +78,53 @@ static Column create( nullable, comment, defaultValue, - /* generationExpression = */ null, + /* columnGenerationExpression = */ null, /* identityColumnSpec = */ null, metadataInJSON, /* id = */ null); } + /** + * Creates a column with a generation expression in SQL string form. + * + * @since 4.1.0 + * @deprecated Use + * {@link #create(String, DataType, boolean, String, GenerationExpression, String)} instead. + */ + @Deprecated Review Comment: The String-form overload of `Column.create` was added in [SPARK-41290](https://issues.apache.org/jira/browse/SPARK-41290) (Feb 2023, ~3.5 era), so adding `@since 4.1.0` on it now is misleading -- `@since` should mark when the method was first introduced, not when it was deprecated. Either drop the `@since` line (matching the original, which had none) or move the version to `@Deprecated(since = "4.1.0")`. ```suggestion * Creates a column with a generation expression in SQL string form. * * @deprecated Use * {@link #create(String, DataType, boolean, String, GenerationExpression, String)} instead. */ @Deprecated(since = "4.1.0") ``` ########## sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala: ########## @@ -1219,6 +1300,16 @@ class DataSourceV2DataFrameSuite } } + private def compareGenerationExpression( + left: GenerationExpression, + right: GenerationExpression): Boolean = { + (left, right) match { + case (null, null) => true + case (null, _) | (_, null) => false + case _ => left.getSql == right.getSql && left.getExpression == right.getExpression + } + } Review Comment: `GenerationExpression` already has structural equality from `SqlOrExpression`, and Scala `==` is null-safe, so this helper is equivalent to `assert(actual == expected)`. Drop the helper and inline at the four call sites (613, 630, 652, 674). ```suggestion ``` ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Column.java: ########## @@ -156,7 +190,25 @@ static Column create( * expression compatibility and reject writes as necessary. */ @Nullable - String generationExpression(); + default String generationExpression() { Review Comment: The default `generationExpression()` here defers to `columnGenerationExpression()` (line 207), and the default `columnGenerationExpression()` defers to `generationExpression()`. A custom `Column` implementer that overrides neither will stack-overflow on the first call -- no clear error. Pre-PR, `generationExpression()` was abstract, which forced implementers to declare it; making it `default` removes that forcing function. The Javadoc says data sources shouldn't implement `Column` directly, but if a connector does and forgets the override, the failure is cryptic. Either keep `generationExpression()` abstract (forces an override, restores the pre-PR safety), or add an explicit Javadoc note: "Implementers must override at least one of `generationExpression()` and `columnGenerationExpression()`." ########## sql/catalyst/src/main/scala/org/apache/spark/sql/connector/catalog/CatalogV2Util.scala: ########## @@ -748,8 +748,14 @@ private[sql] object CatalogV2Util { } else if (isGeneratedColumn) { val cleanedMetadata = metadataWithKeysRemoved( Seq("comment", GeneratedColumn.GENERATION_EXPRESSION_METADATA_KEY)) + val generationExpression = GeneratedColumn.toGenerationExpression( Review Comment: This is the third backward-compat axis the PR design touches: the default `Table.columns()` impl. Before: a stored generation expression survives the StructType → V2 Column roundtrip as a SQL string, no analyzer runs. After: every roundtrip parses + analyzes + re-runs `V2ExpressionBuilder`. Two concrete effects: 1. **Perf**: a table with N generated columns pays N analyzer runs per `Table.columns()` call, and `Table.columns()` is called repeatedly during planning. 2. **Backward-compat break**: a previously-valid stored SQL that no longer analyzes (renamed referenced column, removed function, schema drift) now throws at read time, where pre-PR it passed through as the SQL string and the connector could decide. The `DefaultValue` analogue intentionally doesn't try to recover the V2 form here -- `structFieldToV2Column` constructs `ColumnDefaultValue(sql, value)` with `expr=null`. Connectors that want the V2 form construct it explicitly when they override `Table.columns()` and pass a fully-formed `ColumnDefaultValue(sql, expr, value)` themselves. Suggest matching that pattern: have this branch return `GenerationExpression(sql)` with no V2 expression, and let connectors opt into the V2 form by constructing `GenerationExpression(sql, expr)` themselves via the new `Column.create(..., GenerationExpression, ...)` factory. That preserves the read-path contract without sacrificing the goal of the PR. ########## sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/Column.java: ########## @@ -156,7 +190,25 @@ static Column create( * expression compatibility and reject writes as necessary. */ @Nullable - String generationExpression(); + default String generationExpression() { + return columnGenerationExpression() != null ? columnGenerationExpression().getSql() : null; + } + + /** + * Returns the generation expression of this table column as an {@link GenerationExpression}. + * <p> + * The default implementation wraps {@link #generationExpression()} for backward compatibility + * with custom {@link Column} implementations that only override the string form. New code + * should override this method (or use {@link Column#create} with a {@link GenerationExpression}) + * to also supply a connector {@link org.apache.spark.sql.connector.expressions.Expression}. + * + * @since 4.1.0 + */ + @Nullable Review Comment: Naming: `columnGenerationExpression()` carries a `column` prefix that's pure noise -- the method is already on `Column`, so the prefix qualifies nothing. It exists only to dodge Java's no-overload-by-return-type rule, since `generationExpression()` is taken by the String accessor. Compare with the defaults precedent: - `Column.defaultValue() -> ColumnDefaultValue` -- method name has no prefix; the `Column` qualifier lives on the *type*, where it does real work (distinguishes from the base `DefaultValue` used for SQL variables and ALTER COLUMN payloads). - `Column.columnGenerationExpression() -> GenerationExpression` -- `column` qualifier lives on the *method*, where it does nothing. Asymmetric, and there's no analogous base class to disambiguate against. The cleanest fix would be to make `Column.generationExpression()` return `GenerationExpression` and have callers do `.getSql()` (matches what defaults did from day one), but that's a source break for every connector reading the String accessor. Given the `@Evolving` annotation the break is allowed, but if the author wants to avoid it, **`generationExpressionInfo()`** is a better name than `columnGenerationExpression()`: - The `Info` suffix is a standard Java convention for "wrapper carrying multiple fields" (`TableInfo`, `IdentityColumnSpec`, etc.) -- it tells the reader why a second method exists. - It describes the concept (a generation expression's info), not the receiver. - It removes the false suggestion that "column" is doing work. Optional: rename the type to `GenerationExpressionInfo` too for full symmetry, mirroring `TableInfo`. `DefaultValue` doesn't have the suffix, but it predates this convention and plays a base-class role that `GenerationExpression` doesn't. ```suggestion default GenerationExpression generationExpressionInfo() { ``` (also rename the field on `ColumnImpl`, the call sites in `ColumnDefinition.toV2Column`, and the test call sites.) -- 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]
