[SPARK-45748][SQL] Add a .fromSQL helper function for Literals#43612
[SPARK-45748][SQL] Add a .fromSQL helper function for Literals#43612anchovYu wants to merge 2 commits intoapache:masterfrom
Conversation
anchovYu
changed the title
| } | ||
|
|
||
| val dataType = DataType.fromJson(dataTypeJson) | ||
| val analyzedExpr = if (dataType.sameType(CalendarIntervalType)) { |
There was a problem hiding this comment.
we should check if it contains CalendarIntervalType.
| */ | ||
| private[sql] def fromSQL(valueSqlStr: String, dataTypeJson: String): Literal = { | ||
| def parseAndAnalyze(valueSqlStr: String, dataType: DataType): Expression = { | ||
| lazy val parser = new CatalystSqlParser() |
There was a problem hiding this comment.
| lazy val parser = new CatalystSqlParser() | |
| val parser = new CatalystSqlParser() |
| "PARSE_FAILURE", valueSqlStr, dataType, Some(e)) | ||
| } | ||
|
|
||
| val analyzer: Analyzer = ResolveDefaultColumns.DefaultColumnAnalyzer |
There was a problem hiding this comment.
maybe we can move DefaultColumnAnalyzer to a more common place now.
| throw QueryCompilationErrors.invalidLiteralValueSQLStringForDeserialization( | ||
| "ANALYSIS_FAILURE", valueSqlStr, dataType, Some(e)) | ||
| } | ||
| analyzedPlan.collectFirst { case Project(Seq(a: Alias), OneRowRelation()) => a.child }.get |
There was a problem hiding this comment.
analyzedPlan.asInstanceOf[Project].projectList.head.asInstanceOf[Alias].child
| * @param dataTypeJson the json format data type of the Literal | ||
| * @throws AnalysisException INVALID_LITERAL_VALUE_SQL_STRING_FOR_DESERIALIZATION | ||
| */ | ||
| private[sql] def fromSQL(valueSqlStr: String, dataTypeJson: String): Literal = { |
There was a problem hiding this comment.
shall we use DDL string of the type?
There was a problem hiding this comment.
DDL does not preserves those containsNull fields while json does.
beliefer
left a comment
beliefer
left a comment
There was a problem hiding this comment.
For simplicity, shall we split this PR to two?
|
|
||
|
|
| // ) | ||
| // ) | ||
| // ) | ||
| // ) |
There was a problem hiding this comment.
Could we retain the line starts with // TODO and delete others?
There was a problem hiding this comment.
However the TODO itself is about enabling the commented out test below : )
There was a problem hiding this comment.
You can change it to
TODO(anchovyu): fix the map value comparison problem
| * @throws AnalysisException INVALID_LITERAL_VALUE_SQL_STRING_FOR_DESERIALIZATION | ||
| */ | ||
| private[sql] def fromSQL(valueSqlStr: String, dataTypeJson: String): Literal = { | ||
| def parseAndAnalyze(valueSqlStr: String, dataType: DataType): Expression = { |
There was a problem hiding this comment.
This method is very similar to
Could we reuse the ResolveDefaultColumns.analyze?
There was a problem hiding this comment.
I was thinking about that too but I found that ResolveDefaultColumns.analyze has 1) more column default specific error and error messages 2) has more limitation on the supported case. I also found it can return a non-Literal value as return, e.g. a Cast.
Instead I think in the future ResolveDefaultColumns could refactor to use this general helper function.
| ] | ||
| } | ||
| }, | ||
| "sqlState" : "42894" |
There was a problem hiding this comment.
I this a user error? Shouldn't it be an internal error? I.e. we expect it to work.
There was a problem hiding this comment.
It's a bit tricky that in this PR it's a helper function that accepts any string. It could be a user error that user passes in a random string.
4 participants
What changes were proposed in this pull request?
Two major changes in the PR
fromSQLhelper function for Literal object to parse and analyze the given sql value and data type of the literal, and construct a Literal. Together with.sql, these two methods provide handy function to serialize and deserialize a Literal..sqloutput to adds the timezone information:s"TIMESTAMP '$toString${SQLConf.get.sessionLocalTimeZone}'". This is to help correctly deserialize the value.Why are the changes needed?
Provide handy helper functions.
Does this PR introduce any user-facing change?
One minor change for the
.sqloutput for the TIMESTAMP type.How was this patch tested?
Added new tests.
Was this patch authored or co-authored using generative AI tooling?
No.