iffyio commented on code in PR #1574:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1574#discussion_r1864778051
##########
src/ast/data_type.rs:
##########
@@ -300,7 +311,7 @@ pub enum DataType {
/// [clickhouse]:
https://clickhouse.com/docs/en/sql-reference/data-types/nested-data-structures/nested
Nested(Vec<ColumnDef>),
/// Enums
- Enum(Vec<String>),
+ Enum(Vec<EnumValue>, Option<i64>),
Review Comment:
```suggestion
Enum(Vec<EnumValue>, Option<i64>),
```
is a negative size allowed (wondering if this should be u64 otherwise)?
##########
src/dialect/clickhouse.rs:
##########
@@ -50,4 +50,10 @@ impl Dialect for ClickHouseDialect {
fn supports_limit_comma(&self) -> bool {
true
}
+
+ /// ClickHouse supports `Enum8` and `Enum16` types.
+ /// See
[ClickHouse](https://clickhouse.com/docs/en/sql-reference/data-types/enum)
+ fn supports_enum_type_with_bits(&self) -> bool {
+ true
+ }
Review Comment:
I'm thinking since the syntax isn't at risk of clashing with others we could
have the parser always support it (i.e without gating on dialect)? similar to
other scalar types
##########
src/parser/mod.rs:
##########
@@ -7979,6 +7979,27 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_enum_values(&mut self) -> Result<Vec<EnumValue>, ParserError>
{
+ self.expect_token(&Token::LParen)?;
+ let values = self.parse_comma_separated(Parser::parse_enum_value)?;
+ self.expect_token(&Token::RParen)?;
+ Ok(values)
+ }
+
+ pub fn parse_enum_value(&mut self) -> Result<EnumValue, ParserError> {
+ let str = self.parse_literal_string()?;
+ let value = match self.peek_token().token {
+ Token::Eq => {
+ // Consume the `=` token
+ self.next_token();
+ let value = self.parse_number_value()?;
Review Comment:
it seems from the docs the value can be any integer whereas
`parse_number_value()` doesn't support negative/signed integers, maybe we'd
want to use `parse_number()` here instead?
##########
src/ast/data_type.rs:
##########
@@ -25,10 +25,21 @@ use serde::{Deserialize, Serialize};
#[cfg(feature = "visitor")]
use sqlparser_derive::{Visit, VisitMut};
-use crate::ast::{display_comma_separated, ObjectName, StructField, UnionField};
+use crate::ast::{display_comma_separated, ObjectName, StructField, UnionField,
Value};
use super::{value::escape_single_quote_string, ColumnDef};
+#[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+#[cfg_attr(feature = "visitor", derive(Visit, VisitMut))]
+pub enum EnumValue {
+ String(String),
+ /// ClickHouse allows to specify an integer value for each enum value.
+ ///
+ ///
[clickhouse](https://clickhouse.com/docs/en/sql-reference/data-types/enum)
+ Pair(String, Value),
+}
Review Comment:
```suggestion
pub enum EnumMember {
Name(String),
/// ClickHouse allows to specify an integer value for each enum value.
///
///
[clickhouse](https://clickhouse.com/docs/en/sql-reference/data-types/enum)
NamedValue(String, Value),
}
```
Would something like this be reasonable? thinking member is convention and
the variant would be a bit more descriptive
##########
src/parser/mod.rs:
##########
@@ -7979,6 +7979,27 @@ impl<'a> Parser<'a> {
}
}
+ pub fn parse_enum_values(&mut self) -> Result<Vec<EnumValue>, ParserError>
{
+ self.expect_token(&Token::LParen)?;
+ let values = self.parse_comma_separated(Parser::parse_enum_value)?;
+ self.expect_token(&Token::RParen)?;
+ Ok(values)
+ }
+
+ pub fn parse_enum_value(&mut self) -> Result<EnumValue, ParserError> {
+ let str = self.parse_literal_string()?;
+ let value = match self.peek_token().token {
Review Comment:
This can be simplified as
```rust
if self.consume_token(&Token::Eq) {
Enum::Pair()
} else {
Enum::String()
}
```
one other more of a nit, since the method is small enough and if its only
used once, it could probably be inlined within the `parse_enum_values()`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
