You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by "aokolnychyi (via GitHub)" <gi...@apache.org> on 2025/04/15 16:00:30 UTC
[PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
aokolnychyi opened a new pull request, #50593:
URL: https://github.com/apache/spark/pull/50593
<!--
Thanks for sending a pull request! Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://spark.apache.org/contributing.html
2. Ensure you have added or run the appropriate tests for your PR: https://spark.apache.org/developer-tools.html
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][SPARK-XXXX] Your PR title ...'.
4. Be sure to keep the PR description updated to reflect all changes.
5. Please write your PR title to summarize what this PR proposes.
6. If possible, provide a concise example to reproduce the issue for a faster review.
7. If you want to add a new configuration, please read the guideline first for naming configurations in
'core/src/main/scala/org/apache/spark/internal/config/ConfigEntry.scala'.
8. If you want to add or modify an error type or message, please read the guideline first in
'common/utils/src/main/resources/error/README.md'.
-->
### What changes were proposed in this pull request?
<!--
Please clarify what changes you are proposing. The purpose of this section is to outline the changes and how this PR fixes the issue.
If possible, please consider writing useful notes for better and faster reviews in your PR. See the examples below.
1. If you refactor some codes with changing classes, showing the class hierarchy will help reviewers.
2. If you fix some SQL features, you can provide some references of other DBMSes.
3. If there is design documentation, please add the link.
4. If there is a discussion in the mailing list, please add the link.
-->
This PR makes `ColumnDefaultValue` extend `DefaultValue`.
### Why are the changes needed?
<!--
Please clarify why the changes are needed. For instance,
1. If you propose a new API, clarify the use case for a new API.
2. If you fix a bug, you can clarify why it is a bug.
-->
These changes are needed to pass DSv2 expressions in column defaults in CREATE and REPLACE statements.
### Does this PR introduce _any_ user-facing change?
<!--
Note that it means *any* user-facing change including all aspects such as new features, bug fixes, or other behavior changes. Documentation-only updates are not considered user-facing changes.
If yes, please clarify the previous behavior and the change this PR proposes - provide the console output, description and/or an example to show the behavior difference if possible.
If possible, please also clarify if this is a user-facing change compared to the released Spark versions or within the unreleased branches such as master.
If no, write 'No'.
-->
Yes, but the change is backward compatible.
### How was this patch tested?
<!--
If tests were added, say they were added here. Please make sure to add some test cases that check the changes thoroughly including negative and positive cases if possible.
If it was tested in a way different from regular unit tests, please clarify how you tested step by step, ideally copy and paste-able, so that other reviewers can test and check, and descendants can verify in the future.
If tests were not added, please describe why they were not added and/or why it was difficult to add.
If benchmark tests were added, please run the benchmarks in GitHub Actions for the consistent environment, and the instructions could accord to: https://spark.apache.org/developer-tools.html#github-workflow-benchmarks.
-->
This PR comes with tests.
### Was this patch authored or co-authored using generative AI tooling?
<!--
If generative AI tooling has been used in the process of authoring this patch, please include the
phrase: 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
No.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2047989703
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
@cloud-fan @gengliangwang, does this make sense?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2048303788
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
Hmm, as now it has `Expression`, why we still need `Literal`? Cannot the expression be a literal? Seems it can just have `Expression`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2059397128
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,7 +194,10 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ analyzedChild: Option[Expression] = None)
Review Comment:
Agree with this
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2068859199
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
Okay. Btw, it might be also good to update the document for this class.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2044980720
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -338,4 +341,154 @@ class DataSourceV2DataFrameSuite
Row(2, "unknown")))
}
}
+
+ test("create/replace table with complex foldable default values") {
+ val tableName = "testcat.ns1.ns2.tbl"
+ withTable(tableName) {
+ val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | id INT,
+ | salary INT DEFAULT (100 + 23),
+ | dep STRING DEFAULT ('h' || 'r'),
+ | active BOOLEAN DEFAULT CAST(1 AS BOOLEAN)
+ |) USING foo
+ |""".stripMargin)
+ }
+
+ checkDefaultValues(
+ createExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "(100 + 23)",
+ LiteralValue(123, IntegerType),
Review Comment:
We don't have to covert the original Catalyst expression to V2 in this case as the expression is foldable and doesn't contain expressions like `current_date()` or `current_catalog()`.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2056618614
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -3997,6 +3997,19 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
c.markAsAnalyzed(AnalysisContext.get)
case c: KeepAnalyzedQuery if c.resolved =>
c.storeAnalyzedQuery()
+ case c: CreateTable if c.resolved =>
Review Comment:
I went for handling CREATE/REPLACE directly as these two commands convert defaults to V2. There are a few other commands for dealing with variables that also use `DefaultValueExpression`. I didn't handle them here as there is no need to convert their defaults to V2.
I can make this rule more generic by calling `transformExpressions` on arbitrary commands and defining a separate node pattern for `DefaultValueExpression`. It will be more generic but complicated. I am not sure we need the extra complexity now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2065155777
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
I agree the name is a bit confusing, but I'd prefer to do the rename separately. It will also be limited to internal variables as we can't change public methods. I'd also argue the naming of "initial" and "write" defaults in Iceberg makes a bit more sense than "current" and "exists" we use in Spark.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2049685142
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
@gengliangwang, the timing of the conversion is important. We have a complicated foldable expression initially but it will simplified into a literal by the end of optimization.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2068871979
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,16 +195,29 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
- extends UnaryExpression with Unevaluable {
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ analyzedChild: Option[Expression] = None)
+ extends UnaryExpression
+ with Unevaluable
+ with AnalysisAwareExpression[DefaultValueExpression] {
+
+ final override val nodePatterns: Seq[TreePattern] = Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
+ override def markAsAnalyzed(): DefaultValueExpression =
+ copy(analyzedChild = Some(child))
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val currentDefault = analyzedChild.flatMap(new V2ExpressionBuilder(_).build())
+ val existsDefault = LiteralValue(value, dataType)
+ new ColumnDefaultValue(originalSQL, currentDefault.orNull, existsDefault)
Review Comment:
We don't need `analyzedChild` to be defined? When do we want an un-analyzed `currentDefault` (i.e., it is null)?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #50593:
URL: https://github.com/apache/spark/pull/50593#issuecomment-2807469981
cc @szehon-ho @viirya @cloud-fan @gengliangwang
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2067477571
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -3997,6 +3997,10 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
c.markAsAnalyzed(AnalysisContext.get)
case c: KeepAnalyzedQuery if c.resolved =>
c.storeAnalyzedQuery()
+ case p: LogicalPlan if p.resolved =>
Review Comment:
This is confusing, both the trait name `AnalysisAwareExpression` and the method `markAsAnalyzed`. When the child expression is resolved, the `DefaultValueExpression` is consider resolved. Why do we need this?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2068789274
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -3997,6 +3997,10 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
c.markAsAnalyzed(AnalysisContext.get)
case c: KeepAnalyzedQuery if c.resolved =>
c.storeAnalyzedQuery()
+ case p: LogicalPlan if p.resolved =>
Review Comment:
nvm, I think `AnalysisAwareExpression` is a fine name, as what Spark does is to notify it when it's fully analyzed.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan closed pull request #50593: [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE
URL: https://github.com/apache/spark/pull/50593
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2048303788
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
Hmm, as now it has `Expression`, why we still need `Literal`? Cannot the expression a literal?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2049764640
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
The fragile part is that all expressions that are not evaluable have to implement `FoldableUnevaluable`. It may be true most of the time but not always. Let me think a bit more.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2065157601
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -338,4 +341,166 @@ class DataSourceV2DataFrameSuite
Row(2, "unknown")))
}
}
+
+ test("create/replace table with complex foldable default values") {
+ val tableName = "testcat.ns1.ns2.tbl"
+ withTable(tableName) {
+ val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | id INT,
+ | salary INT DEFAULT (100 + 23),
+ | dep STRING DEFAULT ('h' || 'r'),
+ | active BOOLEAN DEFAULT CAST(1 AS BOOLEAN)
+ |) USING foo
+ |""".stripMargin)
+ }
+
+ checkDefaultValues(
+ createExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "(100 + 23)",
+ new GeneralScalarExpression(
+ "+",
+ Array(LiteralValue(100, IntegerType), LiteralValue(23, IntegerType))),
+ LiteralValue(123, IntegerType)),
+ new ColumnDefaultValue(
+ "('h' || 'r')",
+ new GeneralScalarExpression(
+ "CONCAT",
+ Array(
+ LiteralValue(UTF8String.fromString("h"), StringType),
+ LiteralValue(UTF8String.fromString("r"), StringType))),
+ LiteralValue(UTF8String.fromString("hr"), StringType)),
+ new ColumnDefaultValue(
+ "CAST(1 AS BOOLEAN)",
+ new V2Cast(LiteralValue(1, IntegerType), IntegerType, BooleanType),
+ LiteralValue(true, BooleanType))))
+
+ val df1 = Seq(1).toDF("id")
+ df1.writeTo(tableName).append()
+
+ sql(s"ALTER TABLE $tableName ALTER COLUMN dep SET DEFAULT ('i' || 't')")
+
+ val df2 = Seq(2).toDF("id")
+ df2.writeTo(tableName).append()
+
+ checkAnswer(
+ sql(s"SELECT * FROM $tableName"),
+ Seq(
+ Row(1, 123, "hr", true),
+ Row(2, 123, "it", true)))
+
+ val replaceExec = executeAndKeepPhysicalPlan[ReplaceTableExec] {
+ sql(
+ s"""
+ |REPLACE TABLE $tableName (
+ | id INT,
+ | salary INT DEFAULT (50 * 2),
+ | dep STRING DEFAULT ('un' || 'known'),
+ | active BOOLEAN DEFAULT CAST(0 AS BOOLEAN)
+ |) USING foo
+ |""".stripMargin)
+ }
+
+ checkDefaultValues(
+ replaceExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "(50 * 2)",
+ new GeneralScalarExpression(
+ "*",
+ Array(LiteralValue(50, IntegerType), LiteralValue(2, IntegerType))),
+ LiteralValue(100, IntegerType)),
+ new ColumnDefaultValue(
+ "('un' || 'known')",
+ new GeneralScalarExpression(
+ "CONCAT",
+ Array(
+ LiteralValue(UTF8String.fromString("un"), StringType),
+ LiteralValue(UTF8String.fromString("known"), StringType))),
+ LiteralValue(UTF8String.fromString("unknown"), StringType)),
+ new ColumnDefaultValue(
+ "CAST(0 AS BOOLEAN)",
+ new V2Cast(LiteralValue(0, IntegerType), IntegerType, BooleanType),
+ LiteralValue(false, BooleanType))))
+
+ val df3 = Seq(1).toDF("id")
+ df3.writeTo(tableName).append()
+
+ checkAnswer(
+ sql(s"SELECT * FROM $tableName"),
+ Seq(Row(1, 100, "unknown", false)))
+ }
+ }
+
+ test("create/replace table with current like default values") {
+ val tableName = "testcat.ns1.ns2.tbl"
+ withTable(tableName) {
+ val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
+ sql(s"CREATE TABLE $tableName (id INT, cat STRING DEFAULT current_catalog()) USING foo")
+ }
+
+ checkDefaultValues(
+ createExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "current_catalog()",
+ null, /* no V2 expression */
Review Comment:
Correct, it is not supported yet.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2065159181
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
I reworked this approach to only work with analyzed expressions.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2065157180
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/AnalysisAwareExpression.scala:
##########
@@ -0,0 +1,23 @@
+/*
+ * 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.catalyst.expressions
+
+/** An expression that must be notified when the analysis phase is finished. */
+trait AnalysisAwareExpression[E <: Expression] {
Review Comment:
Making it parametrized is kind of optional. I made it this way so that we don't lose the type info after the copy.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2047940264
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
I am a bit confused too.
```
case class CurrentCatalog()
extends LeafExpression
with DefaultStringProducingExpression
with Unevaluable
```
current_catalog is of `Unevaluable` instead of `FoldableUnevaluable`. From the test case, `exposeCurrentDefaultAsExprV2` will be 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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2080957744
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -338,4 +341,166 @@ class DataSourceV2DataFrameSuite
Row(2, "unknown")))
}
}
+
+ test("create/replace table with complex foldable default values") {
Review Comment:
The new test failed in the Non-ANSI daily test:
- https://github.com/apache/spark/actions/runs/14919514416/job/41912124801
We can reproduce the issue locally through the following approaches:
```
SPARK_ANSI_SQL_MODE=false build/sbt clean "sql/testOnly org.apache.spark.sql.connector.DataSourceV2DataFrameSuite"
```
```
[info] - create/replace table with complex foldable default values *** FAILED *** (39 milliseconds)
[info] ColumnDefaultValue{sql=(100 + 23), expression=null, value=123} did not equal ColumnDefaultValue{sql=(100 + 23), expression=100 + 23, value=123} Default value mismatch for column 'salary': expected ColumnDefaultValue{sql=(100 + 23), expression=100 + 23, value=123} but found ColumnDefaultValue{sql=(100 + 23), expression=null, value=123} (DataSourceV2DataFrameSuite.scala:573)
[info] org.scalatest.exceptions.TestFailedException:
[info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info] at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info] at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$checkDefaultValues$1(DataSourceV2DataFrameSuite.scala:573)
[info] at scala.collection.ArrayOps$.foreach$extension(ArrayOps.scala:1324)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.checkDefaultValues(DataSourceV2DataFrameSuite.scala:571)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$new$46(DataSourceV2DataFrameSuite.scala:382)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info] at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally(SparkErrorUtils.scala:80)
[info] at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally$(SparkErrorUtils.scala:77)
[info] at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:100)
[info] at org.apache.spark.sql.test.SQLTestUtilsBase.withTable(SQLTestUtils.scala:312)
[info] at org.apache.spark.sql.test.SQLTestUtilsBase.withTable$(SQLTestUtils.scala:310)
[info] at org.apache.spark.sql.connector.InsertIntoTests.withTable(InsertIntoTests.scala:42)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$new$45(DataSourceV2DataFrameSuite.scala:348)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info] at org.scalatest.enablers.Timed$$anon$1.timeoutAfter(Timed.scala:127)
[info] at org.scalatest.concurrent.TimeLimits$.failAfterImpl(TimeLimits.scala:282)
[info] at org.scalatest.concurrent.TimeLimits.failAfter(TimeLimits.scala:231)
[info] at org.scalatest.concurrent.TimeLimits.failAfter$(TimeLimits.scala:230)
[info] at org.apache.spark.SparkFunSuite.failAfter(SparkFunSuite.scala:69)
[info] at org.apache.spark.SparkFunSuite.$anonfun$test$2(SparkFunSuite.scala:155)
[info] at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info] at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
[info] at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info] at org.scalatest.Transformer.apply(Transformer.scala:22)
[info] at org.scalatest.Transformer.apply(Transformer.scala:20)
[info] at org.scalatest.funsuite.AnyFunSuiteLike$$anon$1.apply(AnyFunSuiteLike.scala:226)
[info] at org.apache.spark.SparkFunSuite.withFixture(SparkFunSuite.scala:227)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.invokeWithFixture$1(AnyFunSuiteLike.scala:224)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTest$1(AnyFunSuiteLike.scala:236)
[info] at org.scalatest.SuperEngine.runTestImpl(Engine.scala:306)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTest(AnyFunSuiteLike.scala:236)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTest$(AnyFunSuiteLike.scala:218)
[info] at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterEach$$super$runTest(SparkFunSuite.scala:69)
[info] at org.scalatest.BeforeAndAfterEach.runTest(BeforeAndAfterEach.scala:234)
[info] at org.scalatest.BeforeAndAfterEach.runTest$(BeforeAndAfterEach.scala:227)
[info] at org.apache.spark.sql.connector.InsertIntoTests.org$scalatest$BeforeAndAfter$$super$runTest(InsertIntoTests.scala:42)
[info] at org.scalatest.BeforeAndAfter.runTest(BeforeAndAfter.scala:213)
[info] at org.scalatest.BeforeAndAfter.runTest$(BeforeAndAfter.scala:203)
[info] at org.apache.spark.sql.connector.InsertIntoTests.runTest(InsertIntoTests.scala:42)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTests$1(AnyFunSuiteLike.scala:269)
[info] at org.scalatest.SuperEngine.$anonfun$runTestsInBranch$1(Engine.scala:413)
[info] at scala.collection.immutable.List.foreach(List.scala:334)
[info] at org.scalatest.SuperEngine.traverseSubNodes$1(Engine.scala:401)
[info] at org.scalatest.SuperEngine.runTestsInBranch(Engine.scala:396)
[info] at org.scalatest.SuperEngine.runTestsImpl(Engine.scala:475)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTests(AnyFunSuiteLike.scala:269)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTests$(AnyFunSuiteLike.scala:268)
[info] at org.scalatest.funsuite.AnyFunSuite.runTests(AnyFunSuite.scala:1564)
[info] at org.scalatest.Suite.run(Suite.scala:1114)
[info] at org.scalatest.Suite.run$(Suite.scala:1096)
[info] at org.scalatest.funsuite.AnyFunSuite.org$scalatest$funsuite$AnyFunSuiteLike$$super$run(AnyFunSuite.scala:1564)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$run$1(AnyFunSuiteLike.scala:273)
[info] at org.scalatest.SuperEngine.runImpl(Engine.scala:535)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.run(AnyFunSuiteLike.scala:273)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.run$(AnyFunSuiteLike.scala:272)
[info] at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterAll$$super$run(SparkFunSuite.scala:69)
[info] at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:213)
[info] at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
[info] at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
[info] at org.apache.spark.sql.connector.InsertIntoTests.org$scalatest$BeforeAndAfter$$super$run(InsertIntoTests.scala:42)
[info] at org.scalatest.BeforeAndAfter.run(BeforeAndAfter.scala:273)
[info] at org.scalatest.BeforeAndAfter.run$(BeforeAndAfter.scala:271)
[info] at org.apache.spark.sql.connector.InsertIntoTests.run(InsertIntoTests.scala:42)
[info] at org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:321)
[info] at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:517)
[info] at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:414)
[info] at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
[info] at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
[info] at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
[info] at java.base/java.lang.Thread.run(Thread.java:840)
```
@aokolnychyi Do you have time to take a look？ Thanks
also cc @cloud-fan
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2065156488
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -3997,6 +3997,10 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
c.markAsAnalyzed(AnalysisContext.get)
case c: KeepAnalyzedQuery if c.resolved =>
c.storeAnalyzedQuery()
+ case p: LogicalPlan if p.resolved =>
Review Comment:
I can, in theory, limit it to commands, as all current use cases belong to commands, but not sure it is a good idea.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on PR #50593:
URL: https://github.com/apache/spark/pull/50593#issuecomment-2837146229
@cloud-fan @gengliangwang @szehon-ho @viirya, can you take another look?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2065157417
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,7 +194,10 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ analyzedChild: Option[Expression] = None)
Review Comment:
I went back in forth on this in this PR. Updated.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2057250098
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,7 +194,10 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ analyzedChild: Option[Expression] = None)
Review Comment:
do we need to display it? we already display the optimized expression and the original SQL string. We can override `def stringArgs` to exclude this new field in EXPLAIN
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "gengliangwang (via GitHub)" <gi...@apache.org>.
gengliangwang commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2048076949
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
I see. Still, it is a bit tricky to just match `FoldableUnevaluable`.
How about use `V2ExpressionBuilder` to convert the catalyst expression to V2 expression? The current_* is not in the allowed list anyway
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2044978604
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -203,8 +206,19 @@ case class DefaultValueExpression(child: Expression, originalSQL: String)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
Review Comment:
I will work on this in a separate PR.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2046322982
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
shall we allow literals for now to be safe? I don't quite get what `!expr.exists(_.isInstanceOf[FoldableUnevaluable])` means under this context.
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
shall we only allow literals for now to be safe? I don't quite get what `!expr.exists(_.isInstanceOf[FoldableUnevaluable])` means under this context.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2056619992
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -203,7 +206,9 @@ case class DefaultValueExpression(child: Expression, originalSQL: String)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val currentDefault = analyzedChild.flatMap(new V2ExpressionBuilder(_).build())
Review Comment:
@viirya, here is the difference between current and exists default. The current default is not necessarily a literal. It should be just a valid V2 expression.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2056608717
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/expressions/Cast.java:
##########
@@ -57,4 +59,19 @@ public Cast(Expression expression, DataType expressionDataType, DataType targetD
@Override
public Expression[] children() { return new Expression[]{ expression() }; }
+
+ @Override
+ public boolean equals(Object other) {
Review Comment:
Had to fix as my test was failing.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on PR #50593:
URL: https://github.com/apache/spark/pull/50593#issuecomment-2842185753
thanks, merging to master!
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2048310479
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
Hmm, looks like you only handle `Literal` in `DefaultValueExpression.toV2` for `exposeCurrentDefaultAsExprV2 = true` case? Then why not just allow literals here for `exposeCurrentDefaultAsExprV2` like @cloud-fan said?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2049804668
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
But basically in `toV2` method, `exprV2` and `literalV2` are same one, no?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2056618614
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -3997,6 +3997,19 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
c.markAsAnalyzed(AnalysisContext.get)
case c: KeepAnalyzedQuery if c.resolved =>
c.storeAnalyzedQuery()
+ case c: CreateTable if c.resolved =>
Review Comment:
I went for handling CREATE/REPLACE directly as these two commands convert defaults to V2. There are a few other commands for dealing with variables that also use `DefaultValueExpression`. I didn't handle them here as there is no need to convert their defaults to V2. I can make this rule more generic by calling `transformExpressions` on arbitrary commands and defining a separate node pattern for `DefaultValueExpression`. It will be more generic but complicated. I am not sure we need the extra complexity now.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2065155777
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
I agree the name is a bit confusing, but I'd prefer to do the rename separately. It will also be limited to internal variables as we can't change public methods. I'd also argue the naming of "initial" and "write" defaults in Iceberg makes a bit more sense than "current" and "exists".
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2047989560
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,17 +194,32 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ exposeCurrentDefaultAsExprV2: Boolean)
extends UnaryExpression with Unevaluable {
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val literalV2 = LiteralValue(value, dataType)
+ val exprV2 = if (exposeCurrentDefaultAsExprV2) literalV2 else null
+ new ColumnDefaultValue(originalSQL, exprV2, literalV2)
case _ =>
throw QueryCompilationErrors.defaultValueNotConstantError(statement, colName, originalSQL)
}
}
+
+object DefaultValueExpression {
+ def apply(expr: Expression, originalSQL: String): DefaultValueExpression = {
+ // only expose V2 expressions as current defaults if all Catalyst expressions are evaluable
+ // Catalyst expressions like CURRENT_USER or CURRENT_DATE will require special handling
+ val exposeCurrentDefaultAsExprV2 = !expr.exists(_.isInstanceOf[FoldableUnevaluable])
Review Comment:
I believe `Unevaluable` extends `FoldableUnevaluable`, which is very confusing. The idea is that we can optimize and squash all expressions as long as they are not like `CURRENT_USER()`, `CURRENT_CATALOG()`, `NOW`, etc. This is intentional as we don't want to pass a complicated expression to connectors.
Take a look at the example from tests:
```
sql(
s"""
|CREATE TABLE $tableName (
| id INT,
| salary INT DEFAULT (100 + 23),
| dep STRING DEFAULT ('h' || 'r'),
| active BOOLEAN DEFAULT CAST(1 AS BOOLEAN)
|) USING foo
|""".stripMargin)
```
It is fine to fold these expressions and pass the result literal to the connector as a write default. If we pass a simplified version of the expression, there is a higher chance the connector will support that expression.
The following can't be safely squashed into a literal, however:
```
sql(s"CREATE TABLE $tableName (id INT, cat STRING DEFAULT current_catalog()) USING foo")
```
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2080957744
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -338,4 +341,166 @@ class DataSourceV2DataFrameSuite
Row(2, "unknown")))
}
}
+
+ test("create/replace table with complex foldable default values") {
Review Comment:
The new test failed in the Non-ANSI daily test:
- https://github.com/apache/spark/actions/runs/14919514416/job/41912124801
We can reproduce the issue locally through the following approaches:
```
SPARK_ANSI_SQL_MODE=false build/sbt clean "sql/testOnly org.apache.spark.sql.connector.DataSourceV2DataFrameSuite"
```
```
[info] - create/replace table with complex foldable default values *** FAILED *** (39 milliseconds)
[info] ColumnDefaultValue{sql=(100 + 23), expression=null, value=123} did not equal ColumnDefaultValue{sql=(100 + 23), expression=100 + 23, value=123} Default value mismatch for column 'salary': expected ColumnDefaultValue{sql=(100 + 23), expression=100 + 23, value=123} but found ColumnDefaultValue{sql=(100 + 23), expression=null, value=123} (DataSourceV2DataFrameSuite.scala:573)
[info] org.scalatest.exceptions.TestFailedException:
[info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info] at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info] at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$checkDefaultValues$1(DataSourceV2DataFrameSuite.scala:573)
[info] at scala.collection.ArrayOps$.foreach$extension(ArrayOps.scala:1324)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.checkDefaultValues(DataSourceV2DataFrameSuite.scala:571)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$new$46(DataSourceV2DataFrameSuite.scala:382)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info] at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally(SparkErrorUtils.scala:80)
[info] at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally$(SparkErrorUtils.scala:77)
[info] at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:100)
[info] at org.apache.spark.sql.test.SQLTestUtilsBase.withTable(SQLTestUtils.scala:312)
[info] at org.apache.spark.sql.test.SQLTestUtilsBase.withTable$(SQLTestUtils.scala:310)
[info] at org.apache.spark.sql.connector.InsertIntoTests.withTable(InsertIntoTests.scala:42)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$new$45(DataSourceV2DataFrameSuite.scala:348)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info] at org.scalatest.enablers.Timed$$anon$1.timeoutAfter(Timed.scala:127)
[info] at org.scalatest.concurrent.TimeLimits$.failAfterImpl(TimeLimits.scala:282)
[info] at org.scalatest.concurrent.TimeLimits.failAfter(TimeLimits.scala:231)
[info] at org.scalatest.concurrent.TimeLimits.failAfter$(TimeLimits.scala:230)
[info] at org.apache.spark.SparkFunSuite.failAfter(SparkFunSuite.scala:69)
[info] at org.apache.spark.SparkFunSuite.$anonfun$test$2(SparkFunSuite.scala:155)
[info] at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info] at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
[info] at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info] at org.scalatest.Transformer.apply(Transformer.scala:22)
[info] at org.scalatest.Transformer.apply(Transformer.scala:20)
[info] at org.scalatest.funsuite.AnyFunSuiteLike$$anon$1.apply(AnyFunSuiteLike.scala:226)
[info] at org.apache.spark.SparkFunSuite.withFixture(SparkFunSuite.scala:227)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.invokeWithFixture$1(AnyFunSuiteLike.scala:224)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTest$1(AnyFunSuiteLike.scala:236)
[info] at org.scalatest.SuperEngine.runTestImpl(Engine.scala:306)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTest(AnyFunSuiteLike.scala:236)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTest$(AnyFunSuiteLike.scala:218)
[info] at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterEach$$super$runTest(SparkFunSuite.scala:69)
[info] at org.scalatest.BeforeAndAfterEach.runTest(BeforeAndAfterEach.scala:234)
[info] at org.scalatest.BeforeAndAfterEach.runTest$(BeforeAndAfterEach.scala:227)
[info] at org.apache.spark.sql.connector.InsertIntoTests.org$scalatest$BeforeAndAfter$$super$runTest(InsertIntoTests.scala:42)
[info] at org.scalatest.BeforeAndAfter.runTest(BeforeAndAfter.scala:213)
[info] at org.scalatest.BeforeAndAfter.runTest$(BeforeAndAfter.scala:203)
[info] at org.apache.spark.sql.connector.InsertIntoTests.runTest(InsertIntoTests.scala:42)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTests$1(AnyFunSuiteLike.scala:269)
[info] at org.scalatest.SuperEngine.$anonfun$runTestsInBranch$1(Engine.scala:413)
[info] at scala.collection.immutable.List.foreach(List.scala:334)
[info] at org.scalatest.SuperEngine.traverseSubNodes$1(Engine.scala:401)
[info] at org.scalatest.SuperEngine.runTestsInBranch(Engine.scala:396)
[info] at org.scalatest.SuperEngine.runTestsImpl(Engine.scala:475)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTests(AnyFunSuiteLike.scala:269)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTests$(AnyFunSuiteLike.scala:268)
[info] at org.scalatest.funsuite.AnyFunSuite.runTests(AnyFunSuite.scala:1564)
[info] at org.scalatest.Suite.run(Suite.scala:1114)
[info] at org.scalatest.Suite.run$(Suite.scala:1096)
[info] at org.scalatest.funsuite.AnyFunSuite.org$scalatest$funsuite$AnyFunSuiteLike$$super$run(AnyFunSuite.scala:1564)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$run$1(AnyFunSuiteLike.scala:273)
[info] at org.scalatest.SuperEngine.runImpl(Engine.scala:535)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.run(AnyFunSuiteLike.scala:273)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.run$(AnyFunSuiteLike.scala:272)
[info] at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterAll$$super$run(SparkFunSuite.scala:69)
[info] at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:213)
[info] at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
[info] at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
[info] at org.apache.spark.sql.connector.InsertIntoTests.org$scalatest$BeforeAndAfter$$super$run(InsertIntoTests.scala:42)
[info] at org.scalatest.BeforeAndAfter.run(BeforeAndAfter.scala:273)
[info] at org.scalatest.BeforeAndAfter.run$(BeforeAndAfter.scala:271)
[info] at org.apache.spark.sql.connector.InsertIntoTests.run(InsertIntoTests.scala:42)
[info] at org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:321)
[info] at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:517)
[info] at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:414)
[info] at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
[info] at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
[info] at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
[info] at java.base/java.lang.Thread.run(Thread.java:840)
```
@aokolnychyi Do you have time to take a look？ Thanks
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2044980720
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -338,4 +341,154 @@ class DataSourceV2DataFrameSuite
Row(2, "unknown")))
}
}
+
+ test("create/replace table with complex foldable default values") {
+ val tableName = "testcat.ns1.ns2.tbl"
+ withTable(tableName) {
+ val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | id INT,
+ | salary INT DEFAULT (100 + 23),
+ | dep STRING DEFAULT ('h' || 'r'),
+ | active BOOLEAN DEFAULT CAST(1 AS BOOLEAN)
+ |) USING foo
+ |""".stripMargin)
+ }
+
+ checkDefaultValues(
+ createExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "(100 + 23)",
+ LiteralValue(123, IntegerType),
Review Comment:
We don't have to covert the original Catalyst expression to V2 in this case as the expression is foldable and doesn't contain expressions like `current_date()` or `current_catalog()`. It is safe to convert the simplified version of the Catalyst expression.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2045800403
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -68,16 +64,20 @@ public Literal<?> getValue() {
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof ColumnDefaultValue that)) return false;
- return sql.equals(that.sql) && value.equals(that.value);
+ return Objects.equals(getSql(), that.getSql()) &&
+ Objects.equals(getExpression(), that.getExpression()) &&
+ value.equals(that.value);
}
@Override
public int hashCode() {
- return Objects.hash(sql, value);
+ return Objects.hash(getSql(), getExpression(), value);
}
@Override
public String toString() {
- return "ColumnDefaultValue{sql='" + sql + "\', value=" + value + '}';
+ return String.format(
+ "ColumnDefaultValue{sql=%s, expression=%s, value=%s}",
Review Comment:
I removed `''` around sql. We had double quotes for strings. Int literals also showed up as string literals.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2049686345
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
Here `Literal` means the exists or initial literal. It contains the value of the expression evaluated at the time of adding a column to the table.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2056607542
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
They were temporarily, but they can be different. For instance, `NOW()` will be preserved in current default but will be folded into a literal in exists default.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2056619992
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -203,7 +206,9 @@ case class DefaultValueExpression(child: Expression, originalSQL: String)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val currentDefault = analyzedChild.flatMap(new V2ExpressionBuilder(_).build())
Review Comment:
@viirya, here is the difference between current and exists default. The current default is not necessarily a literal.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2057252624
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -338,4 +341,166 @@ class DataSourceV2DataFrameSuite
Row(2, "unknown")))
}
}
+
+ test("create/replace table with complex foldable default values") {
+ val tableName = "testcat.ns1.ns2.tbl"
+ withTable(tableName) {
+ val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
+ sql(
+ s"""
+ |CREATE TABLE $tableName (
+ | id INT,
+ | salary INT DEFAULT (100 + 23),
+ | dep STRING DEFAULT ('h' || 'r'),
+ | active BOOLEAN DEFAULT CAST(1 AS BOOLEAN)
+ |) USING foo
+ |""".stripMargin)
+ }
+
+ checkDefaultValues(
+ createExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "(100 + 23)",
+ new GeneralScalarExpression(
+ "+",
+ Array(LiteralValue(100, IntegerType), LiteralValue(23, IntegerType))),
+ LiteralValue(123, IntegerType)),
+ new ColumnDefaultValue(
+ "('h' || 'r')",
+ new GeneralScalarExpression(
+ "CONCAT",
+ Array(
+ LiteralValue(UTF8String.fromString("h"), StringType),
+ LiteralValue(UTF8String.fromString("r"), StringType))),
+ LiteralValue(UTF8String.fromString("hr"), StringType)),
+ new ColumnDefaultValue(
+ "CAST(1 AS BOOLEAN)",
+ new V2Cast(LiteralValue(1, IntegerType), IntegerType, BooleanType),
+ LiteralValue(true, BooleanType))))
+
+ val df1 = Seq(1).toDF("id")
+ df1.writeTo(tableName).append()
+
+ sql(s"ALTER TABLE $tableName ALTER COLUMN dep SET DEFAULT ('i' || 't')")
+
+ val df2 = Seq(2).toDF("id")
+ df2.writeTo(tableName).append()
+
+ checkAnswer(
+ sql(s"SELECT * FROM $tableName"),
+ Seq(
+ Row(1, 123, "hr", true),
+ Row(2, 123, "it", true)))
+
+ val replaceExec = executeAndKeepPhysicalPlan[ReplaceTableExec] {
+ sql(
+ s"""
+ |REPLACE TABLE $tableName (
+ | id INT,
+ | salary INT DEFAULT (50 * 2),
+ | dep STRING DEFAULT ('un' || 'known'),
+ | active BOOLEAN DEFAULT CAST(0 AS BOOLEAN)
+ |) USING foo
+ |""".stripMargin)
+ }
+
+ checkDefaultValues(
+ replaceExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "(50 * 2)",
+ new GeneralScalarExpression(
+ "*",
+ Array(LiteralValue(50, IntegerType), LiteralValue(2, IntegerType))),
+ LiteralValue(100, IntegerType)),
+ new ColumnDefaultValue(
+ "('un' || 'known')",
+ new GeneralScalarExpression(
+ "CONCAT",
+ Array(
+ LiteralValue(UTF8String.fromString("un"), StringType),
+ LiteralValue(UTF8String.fromString("known"), StringType))),
+ LiteralValue(UTF8String.fromString("unknown"), StringType)),
+ new ColumnDefaultValue(
+ "CAST(0 AS BOOLEAN)",
+ new V2Cast(LiteralValue(0, IntegerType), IntegerType, BooleanType),
+ LiteralValue(false, BooleanType))))
+
+ val df3 = Seq(1).toDF("id")
+ df3.writeTo(tableName).append()
+
+ checkAnswer(
+ sql(s"SELECT * FROM $tableName"),
+ Seq(Row(1, 100, "unknown", false)))
+ }
+ }
+
+ test("create/replace table with current like default values") {
+ val tableName = "testcat.ns1.ns2.tbl"
+ withTable(tableName) {
+ val createExec = executeAndKeepPhysicalPlan[CreateTableExec] {
+ sql(s"CREATE TABLE $tableName (id INT, cat STRING DEFAULT current_catalog()) USING foo")
+ }
+
+ checkDefaultValues(
+ createExec.columns,
+ Array(
+ null,
+ new ColumnDefaultValue(
+ "current_catalog()",
+ null, /* no V2 expression */
Review Comment:
we plan to support these current functions in DS v2, right?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "cloud-fan (via GitHub)" <gi...@apache.org>.
cloud-fan commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2068784295
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala:
##########
@@ -3997,6 +3997,10 @@ class Analyzer(override val catalogManager: CatalogManager) extends RuleExecutor
c.markAsAnalyzed(AnalysisContext.get)
case c: KeepAnalyzedQuery if c.resolved =>
c.storeAnalyzedQuery()
+ case p: LogicalPlan if p.resolved =>
Review Comment:
shall we follow the command name `AnalysisOnlyCommand` and name it `AnalysisOnlyExpression`? The goal is to hide the analyzed children from being optimized.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "aokolnychyi (via GitHub)" <gi...@apache.org>.
aokolnychyi commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2069001807
##########
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/plans/logical/ColumnDefinition.scala:
##########
@@ -194,16 +195,29 @@ object ColumnDefinition {
/**
* A fake expression to hold the column/variable default value expression and its original SQL text.
*/
-case class DefaultValueExpression(child: Expression, originalSQL: String)
- extends UnaryExpression with Unevaluable {
+case class DefaultValueExpression(
+ child: Expression,
+ originalSQL: String,
+ analyzedChild: Option[Expression] = None)
+ extends UnaryExpression
+ with Unevaluable
+ with AnalysisAwareExpression[DefaultValueExpression] {
+
+ final override val nodePatterns: Seq[TreePattern] = Seq(ANALYSIS_AWARE_EXPRESSION)
+
override def dataType: DataType = child.dataType
+ override def stringArgs: Iterator[Any] = Iterator(child, originalSQL)
+ override def markAsAnalyzed(): DefaultValueExpression =
+ copy(analyzedChild = Some(child))
override protected def withNewChildInternal(newChild: Expression): Expression =
copy(child = newChild)
// Convert the default expression to ColumnDefaultValue, which is required by DS v2 APIs.
def toV2(statement: String, colName: String): ColumnDefaultValue = child match {
case Literal(value, dataType) =>
- new ColumnDefaultValue(originalSQL, LiteralValue(value, dataType))
+ val currentDefault = analyzedChild.flatMap(new V2ExpressionBuilder(_).build())
+ val existsDefault = LiteralValue(value, dataType)
+ new ColumnDefaultValue(originalSQL, currentDefault.orNull, existsDefault)
Review Comment:
The V2 expression can be null if a Catalyst expression can't be converted. In that case, the connector has no other option other than to parse the SQL string or fail.
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "LuciferYang (via GitHub)" <gi...@apache.org>.
LuciferYang commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2080957744
##########
sql/core/src/test/scala/org/apache/spark/sql/connector/DataSourceV2DataFrameSuite.scala:
##########
@@ -338,4 +341,166 @@ class DataSourceV2DataFrameSuite
Row(2, "unknown")))
}
}
+
+ test("create/replace table with complex foldable default values") {
Review Comment:
The new test failed in the Non-ANSI daily test:
- https://github.com/apache/spark/actions/runs/14919514416/job/41912124801
Here are the ways we can identify/detect issues locally:
```
SPARK_ANSI_SQL_MODE=false build/sbt clean "sql/testOnly org.apache.spark.sql.connector.DataSourceV2DataFrameSuite"
```
```
[info] - create/replace table with complex foldable default values *** FAILED *** (39 milliseconds)
[info] ColumnDefaultValue{sql=(100 + 23), expression=null, value=123} did not equal ColumnDefaultValue{sql=(100 + 23), expression=100 + 23, value=123} Default value mismatch for column 'salary': expected ColumnDefaultValue{sql=(100 + 23), expression=100 + 23, value=123} but found ColumnDefaultValue{sql=(100 + 23), expression=null, value=123} (DataSourceV2DataFrameSuite.scala:573)
[info] org.scalatest.exceptions.TestFailedException:
[info] at org.scalatest.Assertions.newAssertionFailedException(Assertions.scala:472)
[info] at org.scalatest.Assertions.newAssertionFailedException$(Assertions.scala:471)
[info] at org.scalatest.Assertions$.newAssertionFailedException(Assertions.scala:1231)
[info] at org.scalatest.Assertions$AssertionsHelper.macroAssert(Assertions.scala:1295)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$checkDefaultValues$1(DataSourceV2DataFrameSuite.scala:573)
[info] at scala.collection.ArrayOps$.foreach$extension(ArrayOps.scala:1324)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.checkDefaultValues(DataSourceV2DataFrameSuite.scala:571)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$new$46(DataSourceV2DataFrameSuite.scala:382)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info] at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally(SparkErrorUtils.scala:80)
[info] at org.apache.spark.util.SparkErrorUtils.tryWithSafeFinally$(SparkErrorUtils.scala:77)
[info] at org.apache.spark.util.Utils$.tryWithSafeFinally(Utils.scala:100)
[info] at org.apache.spark.sql.test.SQLTestUtilsBase.withTable(SQLTestUtils.scala:312)
[info] at org.apache.spark.sql.test.SQLTestUtilsBase.withTable$(SQLTestUtils.scala:310)
[info] at org.apache.spark.sql.connector.InsertIntoTests.withTable(InsertIntoTests.scala:42)
[info] at org.apache.spark.sql.connector.DataSourceV2DataFrameSuite.$anonfun$new$45(DataSourceV2DataFrameSuite.scala:348)
[info] at scala.runtime.java8.JFunction0$mcV$sp.apply(JFunction0$mcV$sp.scala:18)
[info] at org.scalatest.enablers.Timed$$anon$1.timeoutAfter(Timed.scala:127)
[info] at org.scalatest.concurrent.TimeLimits$.failAfterImpl(TimeLimits.scala:282)
[info] at org.scalatest.concurrent.TimeLimits.failAfter(TimeLimits.scala:231)
[info] at org.scalatest.concurrent.TimeLimits.failAfter$(TimeLimits.scala:230)
[info] at org.apache.spark.SparkFunSuite.failAfter(SparkFunSuite.scala:69)
[info] at org.apache.spark.SparkFunSuite.$anonfun$test$2(SparkFunSuite.scala:155)
[info] at org.scalatest.OutcomeOf.outcomeOf(OutcomeOf.scala:85)
[info] at org.scalatest.OutcomeOf.outcomeOf$(OutcomeOf.scala:83)
[info] at org.scalatest.OutcomeOf$.outcomeOf(OutcomeOf.scala:104)
[info] at org.scalatest.Transformer.apply(Transformer.scala:22)
[info] at org.scalatest.Transformer.apply(Transformer.scala:20)
[info] at org.scalatest.funsuite.AnyFunSuiteLike$$anon$1.apply(AnyFunSuiteLike.scala:226)
[info] at org.apache.spark.SparkFunSuite.withFixture(SparkFunSuite.scala:227)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.invokeWithFixture$1(AnyFunSuiteLike.scala:224)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTest$1(AnyFunSuiteLike.scala:236)
[info] at org.scalatest.SuperEngine.runTestImpl(Engine.scala:306)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTest(AnyFunSuiteLike.scala:236)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTest$(AnyFunSuiteLike.scala:218)
[info] at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterEach$$super$runTest(SparkFunSuite.scala:69)
[info] at org.scalatest.BeforeAndAfterEach.runTest(BeforeAndAfterEach.scala:234)
[info] at org.scalatest.BeforeAndAfterEach.runTest$(BeforeAndAfterEach.scala:227)
[info] at org.apache.spark.sql.connector.InsertIntoTests.org$scalatest$BeforeAndAfter$$super$runTest(InsertIntoTests.scala:42)
[info] at org.scalatest.BeforeAndAfter.runTest(BeforeAndAfter.scala:213)
[info] at org.scalatest.BeforeAndAfter.runTest$(BeforeAndAfter.scala:203)
[info] at org.apache.spark.sql.connector.InsertIntoTests.runTest(InsertIntoTests.scala:42)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$runTests$1(AnyFunSuiteLike.scala:269)
[info] at org.scalatest.SuperEngine.$anonfun$runTestsInBranch$1(Engine.scala:413)
[info] at scala.collection.immutable.List.foreach(List.scala:334)
[info] at org.scalatest.SuperEngine.traverseSubNodes$1(Engine.scala:401)
[info] at org.scalatest.SuperEngine.runTestsInBranch(Engine.scala:396)
[info] at org.scalatest.SuperEngine.runTestsImpl(Engine.scala:475)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTests(AnyFunSuiteLike.scala:269)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.runTests$(AnyFunSuiteLike.scala:268)
[info] at org.scalatest.funsuite.AnyFunSuite.runTests(AnyFunSuite.scala:1564)
[info] at org.scalatest.Suite.run(Suite.scala:1114)
[info] at org.scalatest.Suite.run$(Suite.scala:1096)
[info] at org.scalatest.funsuite.AnyFunSuite.org$scalatest$funsuite$AnyFunSuiteLike$$super$run(AnyFunSuite.scala:1564)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.$anonfun$run$1(AnyFunSuiteLike.scala:273)
[info] at org.scalatest.SuperEngine.runImpl(Engine.scala:535)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.run(AnyFunSuiteLike.scala:273)
[info] at org.scalatest.funsuite.AnyFunSuiteLike.run$(AnyFunSuiteLike.scala:272)
[info] at org.apache.spark.SparkFunSuite.org$scalatest$BeforeAndAfterAll$$super$run(SparkFunSuite.scala:69)
[info] at org.scalatest.BeforeAndAfterAll.liftedTree1$1(BeforeAndAfterAll.scala:213)
[info] at org.scalatest.BeforeAndAfterAll.run(BeforeAndAfterAll.scala:210)
[info] at org.scalatest.BeforeAndAfterAll.run$(BeforeAndAfterAll.scala:208)
[info] at org.apache.spark.sql.connector.InsertIntoTests.org$scalatest$BeforeAndAfter$$super$run(InsertIntoTests.scala:42)
[info] at org.scalatest.BeforeAndAfter.run(BeforeAndAfter.scala:273)
[info] at org.scalatest.BeforeAndAfter.run$(BeforeAndAfter.scala:271)
[info] at org.apache.spark.sql.connector.InsertIntoTests.run(InsertIntoTests.scala:42)
[info] at org.scalatest.tools.Framework.org$scalatest$tools$Framework$$runSuite(Framework.scala:321)
[info] at org.scalatest.tools.Framework$ScalaTestTask.execute(Framework.scala:517)
[info] at sbt.ForkMain$Run.lambda$runTest$1(ForkMain.java:414)
[info] at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
[info] at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
[info] at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
[info] at java.base/java.lang.Thread.run(Thread.java:840)
```
@aokolnychyi Do you have time to take a look？ Thanks
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
Re: [PR] [SPARK-51786][SQL] DSv2 expressions in column defaults in CREATE/REPLACE [spark]
Posted by "szehon-ho (via GitHub)" <gi...@apache.org>.
szehon-ho commented on code in PR #50593:
URL: https://github.com/apache/spark/pull/50593#discussion_r2059395963
##########
sql/catalyst/src/main/java/org/apache/spark/sql/connector/catalog/ColumnDefaultValue.java:
##########
@@ -33,25 +34,20 @@
* data that do not have these new columns.
*/
@Evolving
-public class ColumnDefaultValue {
- private String sql;
- private Literal<?> value;
+public class ColumnDefaultValue extends DefaultValue {
+ private final Literal<?> value;
public ColumnDefaultValue(String sql, Literal<?> value) {
- this.sql = sql;
- this.value = value;
+ this(sql, null /* no expression */, value);
}
- /**
- * Returns the SQL string (Spark SQL dialect) of the default value expression. This is the
- * original string contents of the SQL expression specified at the time the column was created in
- * a CREATE TABLE, REPLACE TABLE, or ADD COLUMN command. For example, for
- * "CREATE TABLE t (col INT DEFAULT 40 + 2)", this returns the string literal "40 + 2" (without
- * quotation marks).
- */
- @Nonnull
- public String getSql() {
- return sql;
+ public ColumnDefaultValue(Expression expr, Literal<?> value) {
+ this(null /* no sql */, expr, value);
+ }
+
+ public ColumnDefaultValue(String sql, Expression expr, Literal<?> value) {
Review Comment:
it is quite confusing. Can we call value => 'existsDefault' to align with the ResolveDefaultColumnsUtils constants?
--
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: reviews-unsubscribe@spark.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org