Skip to content

[WIP][SPARK-54652] Complete conversion of IDENTIFIER() #53407

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
aleksandr-chernousov-db wants to merge 6 commits into
base: master
Choose a base branch
from
+1,493 −49
Open

[WIP][SPARK-54652] Complete conversion of IDENTIFIER() #53407

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d42d3a
Fix usage of identifiers
Dec 9, 2025
d5f8475
add tests
Dec 9, 2025
6433504
Update tests
Dec 9, 2025
72db2e8
Return getText for identifiers used as placeholders for keywords
aleksandr-chernousov-db Dec 10, 2025
3d6ad1c
fix fmt
aleksandr-chernousov-db Dec 10, 2025
cc82512
tests for keywords
aleksandr-chernousov-db Dec 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -497,7 +497,7 @@ class DataTypeAstBuilder extends SqlBaseParserBaseVisitor[AnyRef] with DataTypeE
override def visitComplexColType(ctx: ComplexColTypeContext): StructField = withOrigin(ctx) {
import ctx._
val structField = StructField(
name = errorCapturingIdentifier.getText,
name = getIdentifierText(errorCapturingIdentifier),
dataType = typedVisit(dataType()),
nullable = NULL == null)
Option(commentSpec).map(visitCommentSpec).map(structField.withComment).getOrElse(structField)
Expand Down
36 changes: 18 additions & 18 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/parser/AstBuilder.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1235,7 +1235,7 @@ class AstBuilder extends DataTypeAstBuilder
if (pVal.DEFAULT != null) {
throw QueryParsingErrors.defaultColumnReferencesNotAllowedInPartitionSpec(ctx)
}
val name = pVal.identifier.getText
val name = getIdentifierText(pVal.identifier)
val value = Option(pVal.constant).map(v => {
visitStringConstant(v, legacyNullAsString, keepPartitionSpecAsString)
})
Expand Down Expand Up @@ -1958,11 +1958,11 @@ class AstBuilder extends DataTypeAstBuilder
.flatMap(_.namedExpression.asScala)
.map(typedVisit[Expression])
val pivotColumn = if (ctx.pivotColumn.identifiers.size == 1) {
UnresolvedAttribute.quoted(ctx.pivotColumn.errorCapturingIdentifier.getText)
UnresolvedAttribute.quoted(getIdentifierText(ctx.pivotColumn.errorCapturingIdentifier))
} else {
CreateStruct(
ctx.pivotColumn.identifiers.asScala.map(
identifier => UnresolvedAttribute.quoted(identifier.getText)).toSeq)
identifier => UnresolvedAttribute.quoted(getIdentifierText(identifier))).toSeq)
}
val pivotValues = ctx.pivotValues.asScala.map(visitPivotValue)
Pivot(None, pivotColumn, pivotValues.toSeq, aggregates, query)
Expand All @@ -1974,7 +1974,7 @@ class AstBuilder extends DataTypeAstBuilder
override def visitPivotValue(ctx: PivotValueContext): Expression = withOrigin(ctx) {
val e = expression(ctx.expression)
if (ctx.errorCapturingIdentifier != null) {
Alias(e, ctx.errorCapturingIdentifier.getText)()
Alias(e, getIdentifierText(ctx.errorCapturingIdentifier))()
} else {
e
}
Expand Down Expand Up @@ -2039,7 +2039,7 @@ class AstBuilder extends DataTypeAstBuilder

// alias unpivot result
if (ctx.errorCapturingIdentifier() != null) {
val alias = ctx.errorCapturingIdentifier().getText
val alias = getIdentifierText(ctx.errorCapturingIdentifier())
SubqueryAlias(alias, filtered)
} else {
filtered
Expand Down Expand Up @@ -2541,7 +2541,7 @@ class AstBuilder extends DataTypeAstBuilder
*/
private def mayApplyAliasPlan(tableAlias: TableAliasContext, plan: LogicalPlan): LogicalPlan = {
if (tableAlias.strictIdentifier != null) {
val alias = tableAlias.strictIdentifier.getText
val alias = getIdentifierText(tableAlias.strictIdentifier)
if (tableAlias.identifierList != null) {
val columnNames = visitIdentifierList(tableAlias.identifierList)
SubqueryAlias(alias, UnresolvedSubqueryColumnAliases(columnNames, plan))
Expand Down Expand Up @@ -3229,7 +3229,7 @@ class AstBuilder extends DataTypeAstBuilder
*/
override def visitLambda(ctx: LambdaContext): Expression = withOrigin(ctx) {
val arguments = ctx.identifier().asScala.map { name =>
UnresolvedNamedLambdaVariable(UnresolvedAttribute.quoted(name.getText).nameParts)
UnresolvedNamedLambdaVariable(UnresolvedAttribute.quoted(getIdentifierText(name)).nameParts)
}
val function = expression(ctx.expression).transformUp {
case a: UnresolvedAttribute => UnresolvedNamedLambdaVariable(a.nameParts)
Expand Down Expand Up @@ -4261,7 +4261,7 @@ class AstBuilder extends DataTypeAstBuilder
if (!SQLConf.get.objectLevelCollationsEnabled) {
throw QueryCompilationErrors.objectLevelCollationsNotEnabledError()
}
val collationName = ctx.identifier.getText
val collationName = getIdentifierText(ctx.identifier)
CollationFactory.fetchCollation(collationName).collationName
}

Expand Down Expand Up @@ -4500,7 +4500,7 @@ class AstBuilder extends DataTypeAstBuilder
def getFieldReference(
ctx: ApplyTransformContext,
arg: V2Expression): FieldReference = {
lazy val name: String = ctx.identifier.getText
lazy val name: String = getIdentifierText(ctx.identifier)
arg match {
case ref: FieldReference =>
ref
Expand All @@ -4512,7 +4512,7 @@ class AstBuilder extends DataTypeAstBuilder
def getSingleFieldReference(
ctx: ApplyTransformContext,
arguments: Seq[V2Expression]): FieldReference = {
lazy val name: String = ctx.identifier.getText
lazy val name: String = getIdentifierText(ctx.identifier)
if (arguments.size > 1) {
throw QueryParsingErrors.wrongNumberArgumentsForTransformError(name, arguments.size, ctx)
} else if (arguments.isEmpty) {
Expand All @@ -4529,7 +4529,7 @@ class AstBuilder extends DataTypeAstBuilder
case applyCtx: ApplyTransformContext =>
val arguments = applyCtx.argument.asScala.map(visitTransformArgument).toSeq

applyCtx.identifier.getText match {
getIdentifierText(applyCtx.identifier) match {
case "bucket" =>
val numBuckets: Int = arguments.head match {
case LiteralValue(shortValue, ShortType) =>
Expand Down Expand Up @@ -4797,7 +4797,7 @@ class AstBuilder extends DataTypeAstBuilder
string(visitStringLit(c.outFmt)))))
// Expected format: SEQUENCEFILE | TEXTFILE | RCFILE | ORC | PARQUET | AVRO
case (c: GenericFileFormatContext, null) =>
SerdeInfo(storedAs = Some(c.identifier.getText))
SerdeInfo(storedAs = Some(getIdentifierText(c.identifier)))
case (null, storageHandler) =>
invalidStatement("STORED BY", ctx)
case _ =>
Expand Down Expand Up @@ -4887,15 +4887,15 @@ class AstBuilder extends DataTypeAstBuilder
(rowFormatCtx, createFileFormatCtx.fileFormat) match {
case (_, ffTable: TableFileFormatContext) => // OK
case (rfSerde: RowFormatSerdeContext, ffGeneric: GenericFileFormatContext) =>
ffGeneric.identifier.getText.toLowerCase(Locale.ROOT) match {
getIdentifierText(ffGeneric.identifier).toLowerCase(Locale.ROOT) match {
case ("sequencefile" | "textfile" | "rcfile") => // OK
case fmt =>
operationNotAllowed(
s"ROW FORMAT SERDE is incompatible with format '$fmt', which also specifies a serde",
parentCtx)
}
case (rfDelimited: RowFormatDelimitedContext, ffGeneric: GenericFileFormatContext) =>
ffGeneric.identifier.getText.toLowerCase(Locale.ROOT) match {
getIdentifierText(ffGeneric.identifier).toLowerCase(Locale.ROOT) match {
Copy link
Author

@aleksandr-chernousov-db aleksandr-chernousov-db Dec 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose here is also a keyword, not an identifier?

Copy link
Contributor

@srielau srielau Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct the keyword appears to be TEXTFILE

case "textfile" => // OK
case fmt => operationNotAllowed(
s"ROW FORMAT DELIMITED is only compatible with 'textfile', not '$fmt'", parentCtx)
Expand Down Expand Up @@ -5838,7 +5838,7 @@ class AstBuilder extends DataTypeAstBuilder
}
}
if (ctx.identifier != null &&
ctx.identifier.getText.toLowerCase(Locale.ROOT) != "noscan") {
getIdentifierText(ctx.identifier).toLowerCase(Locale.ROOT) != "noscan") {
throw QueryParsingErrors.computeStatisticsNotExpectedError(ctx.identifier())
}

Expand Down Expand Up @@ -5879,7 +5879,7 @@ class AstBuilder extends DataTypeAstBuilder
*/
override def visitAnalyzeTables(ctx: AnalyzeTablesContext): LogicalPlan = withOrigin(ctx) {
if (ctx.identifier != null &&
ctx.identifier.getText.toLowerCase(Locale.ROOT) != "noscan") {
getIdentifierText(ctx.identifier).toLowerCase(Locale.ROOT) != "noscan") {
throw QueryParsingErrors.computeStatisticsNotExpectedError(ctx.identifier())
}
val ns = if (ctx.identifierReference() != null) {
Expand Down Expand Up @@ -6433,7 +6433,7 @@ class AstBuilder extends DataTypeAstBuilder
* }}}
*/
override def visitDropIndex(ctx: DropIndexContext): LogicalPlan = withOrigin(ctx) {
val indexName = ctx.identifier.getText
val indexName = getIdentifierText(ctx.identifier)
DropIndex(
createUnresolvedTable(ctx.identifierReference, "DROP INDEX"),
indexName,
Expand Down Expand Up @@ -6655,7 +6655,7 @@ class AstBuilder extends DataTypeAstBuilder
target = None, excepts = ids.map(s => Seq(s)), replacements = None))
Project(projectList, left)
}.getOrElse(Option(ctx.AS).map { _ =>
SubqueryAlias(ctx.errorCapturingIdentifier().getText, left)
SubqueryAlias(getIdentifierText(ctx.errorCapturingIdentifier()), left)
}.getOrElse(Option(ctx.whereClause).map { c =>
if (ctx.windowClause() != null) {
throw QueryParsingErrors.windowClauseInPipeOperatorWhereClauseNotAllowedError(ctx)
Expand Down
10 changes: 5 additions & 5 deletions sql/core/src/main/scala/org/apache/spark/sql/execution/SparkSqlParser.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ class SparkSqlAstBuilder extends AstBuilder {
(ident, _) => builder(ident))
} else if (ctx.errorCapturingIdentifier() != null) {
// resolve immediately
builder.apply(Seq(ctx.errorCapturingIdentifier().getText))
builder.apply(Seq(getIdentifierText(ctx.errorCapturingIdentifier())))
} else if (ctx.stringLit() != null) {
// resolve immediately
builder.apply(Seq(string(visitStringLit(ctx.stringLit()))))
Expand Down Expand Up @@ -575,14 +575,14 @@ class SparkSqlAstBuilder extends AstBuilder {

ctx.op.getType match {
case SqlBaseParser.ADD =>
ctx.identifier.getText.toLowerCase(Locale.ROOT) match {
getIdentifierText(ctx.identifier).toLowerCase(Locale.ROOT) match {
case "files" | "file" => AddFilesCommand(maybePaths)
case "jars" | "jar" => AddJarsCommand(maybePaths)
case "archives" | "archive" => AddArchivesCommand(maybePaths)
case other => operationNotAllowed(s"ADD with resource type '$other'", ctx)
}
case SqlBaseParser.LIST =>
ctx.identifier.getText.toLowerCase(Locale.ROOT) match {
getIdentifierText(ctx.identifier()).toLowerCase(Locale.ROOT) match {
case "files" | "file" =>
if (maybePaths.length > 0) {
ListFilesCommand(maybePaths)
Expand Down Expand Up @@ -735,7 +735,7 @@ class SparkSqlAstBuilder extends AstBuilder {
*/
override def visitCreateFunction(ctx: CreateFunctionContext): LogicalPlan = withOrigin(ctx) {
val resources = ctx.resource.asScala.map { resource =>
val resourceType = resource.identifier.getText.toLowerCase(Locale.ROOT)
val resourceType = getIdentifierText(resource.identifier).toLowerCase(Locale.ROOT)
resourceType match {
case "jar" | "file" | "archive" =>
FunctionResource(FunctionResourceType.fromString(resourceType),
Expand Down Expand Up @@ -1308,7 +1308,7 @@ class SparkSqlAstBuilder extends AstBuilder {
} else {
DescribeColumn(
relation,
UnresolvedAttribute(ctx.describeColName.nameParts.asScala.map(_.getText).toSeq),
UnresolvedAttribute(ctx.describeColName.nameParts.asScala.map(getIdentifierText).toSeq),
isExtended)
}
} else {
Expand Down
Loading