[SPARK-19830] [SQL] Add parseTableSchema API to ParserInterface #17171
Conversation
|
Test build #73965 has finished for PR 17171 at commit
|
| } | ||
| } | ||
|
|
||
| checkTableSchema("a int", (new StructType).add("a", "int")) |
There was a problem hiding this comment.
nit: new StructType().add...
| val e = intercept[ParseException] { | ||
| CatalystSqlParser.parseTableSchema("a INT b long") | ||
| }.getMessage | ||
| assert(e.contains("mismatched input 'b' expecting {<EOF>, '(', ',', 'COMMENT'}")) |
There was a problem hiding this comment.
I am not sure we should check this. This is an ANTLR error message.
| def parseTableIdentifier(sqlText: String): TableIdentifier | ||
|
|
||
| /** Creates StructType for a given SQL string. */ | ||
| def parseTableSchema(sqlText: String): StructType |
There was a problem hiding this comment.
So the difference between the parseDataType and the parseTableSchema functions is that the latter allows you to parse a comma separated list of field definitions which will preserve the correct Hive metadata? Could you document this?
| * Create top level table schema. | ||
| */ | ||
| protected def createSchema(ctx: ColTypeListContext): StructType = { | ||
| def createSchema(ctx: ColTypeListContext): StructType = { |
There was a problem hiding this comment.
We need to be a little bit careful here. I think we may need to add a rule similar to singleDataType to the grammar and expose its implementation. This to prevent us from silently dropping trailing input, or having a weird error on trailing input; it seems we get a somewhat vague error ATM...
| * definitions which will preserve the correct Hive metadata. | ||
| */ | ||
| override def parseTableSchema(sqlText: String): StructType = parse(sqlText) { parser => | ||
| StructType(astBuilder.visitColTypeList(parser.colTypeList())) |
There was a problem hiding this comment.
@hvanhovell How about this? I am not sure whether I address your concern. Feel free to let me know if you have any concern
|
Test build #74048 has finished for PR 17171 at commit
|
|
Test build #74052 has finished for PR 17171 at commit
|
|
retest this please |
|
Test build #74390 has finished for PR 17171 at commit
|
|
ping @hvanhovell @cloud-fan |
| def parse(sql: String): StructType = CatalystSqlParser.parseTableSchema(sql) | ||
|
|
||
| def checkTableSchema(tableSchemaString: String, expectedDataType: DataType): Unit = { | ||
| test(s"parse ${tableSchemaString.replace("\n", "")}") { |
There was a problem hiding this comment.
seems tableSchemaString will never contains \n?
| } | ||
| } | ||
|
|
||
| def intercept(sql: String): Unit = |
There was a problem hiding this comment.
I'd like to name it assertError
|
LGTM |
|
Test build #74635 has finished for PR 17171 at commit
|
|
thanks, merging to master! |
4 participants
What changes were proposed in this pull request?
Specifying the table schema in DDL formats is needed for different scenarios. For example,
from_jsonusing DDL formats, which is suggested by @marmbrus ,These two PRs need users to use the JSON format to specify the table schema. This is not user friendly.
This PR is to provide a
parseTableSchemaAPI inParserInterface.How was this patch tested?
Added a test suite
TableSchemaParserSuite