mvzink commented on code in PR #1747:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1747#discussion_r1991906767
##########
src/parser/mod.rs:
##########
@@ -6928,13 +6874,122 @@ impl<'a> Parser<'a> {
};
}
+ let plain_options = self.parse_plain_options()?;
+
Ok(CreateTableConfiguration {
partition_by,
cluster_by,
options,
+ plain_options,
})
}
+ fn parse_plain_option(&mut self) -> Result<Option<SqlOption>, ParserError>
{
+ if let Some(option) = self.dialect.parse_plain_option(self)? {
+ return Ok(Some(option));
+ }
+
+ // Parse table options in any order
+ if let Some(keyword) = self.parse_one_of_keywords(&[
+ Keyword::ENGINE,
+ Keyword::AUTO_INCREMENT,
+ Keyword::DEFAULT,
+ Keyword::CHARSET,
+ Keyword::COLLATE,
+ Keyword::INSERT_METHOD,
+ ]) {
+ let _ = self.consume_token(&Token::Eq);
+ let value = self.next_token();
+
+ match (keyword, value.token) {
+ (Keyword::AUTO_INCREMENT, Token::Number(n, l)) => {
Review Comment:
I think this is missing the optional `=` parsing that was present in the old
version (and I believe was correct).
##########
src/ast/helpers/stmt_create_table.rs:
##########
@@ -78,25 +79,22 @@ pub struct CreateTableBuilder {
pub hive_formats: Option<HiveFormat>,
pub table_properties: Vec<SqlOption>,
pub with_options: Vec<SqlOption>,
+ pub options: Option<Vec<SqlOption>>,
+ pub plain_options: Vec<SqlOption>,
pub file_format: Option<FileFormat>,
pub location: Option<String>,
pub query: Option<Box<Query>>,
pub without_rowid: bool,
pub like: Option<ObjectName>,
pub clone: Option<ObjectName>,
- pub engine: Option<TableEngine>,
pub comment: Option<CommentDef>,
Review Comment:
I wonder if `comment` should be folded in to options as well (with a new
`SqlOption::Comment(CommentDef)` variant)?
##########
src/ast/mod.rs:
##########
@@ -6925,6 +6928,12 @@ pub enum SqlOption {
range_direction: Option<PartitionRangeDirection>,
for_values: Vec<Expr>,
},
+
+ TableSpace(TablespaceOption),
+
+ Union(Vec<Ident>),
+
+ TableEngine(TableEngine),
Review Comment:
tbh I haven't checked where else `SqlOption` is used, but it really seems
like it should be called `TableOption`, no?
##########
src/parser/mod.rs:
##########
@@ -6928,13 +6874,122 @@ impl<'a> Parser<'a> {
};
}
+ let plain_options = self.parse_plain_options()?;
+
Ok(CreateTableConfiguration {
partition_by,
cluster_by,
options,
+ plain_options,
})
}
+ fn parse_plain_option(&mut self) -> Result<Option<SqlOption>, ParserError>
{
+ if let Some(option) = self.dialect.parse_plain_option(self)? {
+ return Ok(Some(option));
+ }
+
+ // Parse table options in any order
+ if let Some(keyword) = self.parse_one_of_keywords(&[
+ Keyword::ENGINE,
+ Keyword::AUTO_INCREMENT,
+ Keyword::DEFAULT,
+ Keyword::CHARSET,
+ Keyword::COLLATE,
+ Keyword::INSERT_METHOD,
+ ]) {
Review Comment:
I'm not sure I understand the motivation for making this a dialect parsing
method. Is there a problem with putting all the supported options across all
dialects in here? It especially raises my eyebrow to see the two lists have so
much overlap, including with (afaik) MySQL-specific options like `ENGINE` and
`AUTO_INCREMENT` in the generic set. If we are going to have Postgres and MSSQL
parse those, why not have them parse `DATA DIRECTORY` too?
--
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: github-unsubscr...@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscr...@datafusion.apache.org
For additional commands, e-mail: github-h...@datafusion.apache.org
