[SPARK-52706][SQL] Fix inconsistencies and refactor primitive types in parser #51335
Conversation
|
@MaxGekk @cloud-fan Could you take a look at this PR? We should aim to keep our parser in the most readable and most efficient state. |
| primitiveType | ||
| : primitiveTypeWithParameters | ||
| | primitiveTypeWithoutParameters | ||
| | unsupportedType=identifier (LEFT_PAREN INTEGER_VALUE(COMMA INTEGER_VALUE)* RIGHT_PAREN)? |
There was a problem hiding this comment.
what if we get an unsupportedType with a suffix like: TIME WITH TIME ZONE?
There was a problem hiding this comment.
Yeah, this potentially can make a problem. I mean, we would probably return a bad error message. Let me think if we can scope issues like this as well.
There was a problem hiding this comment.
Actually error message will stay the same, even previously we would return syntax error. The only thing here is if we want to improve the error messages a bit further?
| @@ -1340,7 +1340,20 @@ collateClause | |||
| : COLLATE collationName=multipartIdentifier | |||
| ; | |||
|
|
|||
| type | |||
| primitiveTypeWithParameters | |||
| : STRING collateClause? | |||
There was a problem hiding this comment.
should this be primitiveTypeWithoutParameters?
There was a problem hiding this comment.
It can go, but in this case I would say collation is a parameter as well. It can change it's value to some different value not known at parsing time. If we follow this case, then probably INTERVAL should go to primitiveTypeWithoutParameters, as it is actually without parameters.
There was a problem hiding this comment.
By not known at parsing time, I mean identifier/arbitrary value.
mihailom-db
changed the title
| | BINARY | ||
| | DECIMAL | DEC | NUMERIC | ||
| | VOID | ||
| | INTERVAL |
There was a problem hiding this comment.
shall we move this one into nonTrivialPrimitiveType?
There was a problem hiding this comment.
Moved both INTERVAL and TIMESTAMP so that in the code it is clear that trivial types do not require post processing, but only return a specific type.
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala
Outdated
Show resolved
Hide resolved
sql/api/src/main/scala/org/apache/spark/sql/catalyst/parser/DataTypeAstBuilder.scala
Outdated
Show resolved
Hide resolved
| @@ -165,6 +183,9 @@ class DataTypeAstBuilder extends SqlBaseParserBaseVisitor[AnyRef] { | |||
| * Create a complex DataType. Arrays, Maps and Structures are supported. | |||
| */ | |||
| override def visitComplexDataType(ctx: ComplexDataTypeContext): DataType = withOrigin(ctx) { | |||
| if (ctx.LT() == null && ctx.NEQ() == null) { | |||
| throw QueryParsingErrors.nestedTypeMissingElementTypeError(ctx.getText, ctx) | |||
There was a problem hiding this comment.
No, this is refactoring that I did. Previously if someone only writes STRUCT/ARRAY/MAP without parameters it would go to path of the primitive type. This is not a good practice, so I made a change that complex types are isolated in the separate context. The only change here is that we would only change the error message for when someone writes STRUCT(2) where it would return unsupported primitive type instead of the complex type missing element. We could argue that this is a change, but if you ask me, we need to distinguish between primitive and complex types first, as this is a general practice in type theory. We have primitive types which can be used as leaf arguments in complex types, we do not want to go into some recursive link between the two.
What changes were proposed in this pull request?
This PR proposes a change in how our parser treats datatypes. We introduce types with/without parameters and group accordingly.
Why are the changes needed?
Changes are needed for many reasons:
Does this PR introduce any user-facing change?
No. This is internal refactoring.
How was this patch tested?
All existing tests should pass, this is just code refactoring.
Was this patch authored or co-authored using generative AI tooling?
No.