iffyio commented on code in PR #1472:
URL:
https://github.com/apache/datafusion-sqlparser-rs/pull/1472#discussion_r1806474198
##########
src/parser/mod.rs:
##########
@@ -2796,9 +2811,27 @@ impl<'a> Parser<'a> {
format: None,
})
} else if Token::ExclamationMark == tok {
- // PostgreSQL factorial operation
+ match expr {
+ Expr::Value(_) | Expr::Identifier(_) => {
+ if !self.dialect.supports_factorial_operator() {
+ return parser_err!(
+ format!(
+ "current dialect: {:?} does not support
factorial operator",
+ self.dialect
+ ),
+ self.peek_token().location
+ );
+ }
+ }
+ _ => {}
+ };
+ let op = if !self.dialect.supports_factorial_operator() {
+ UnaryOperator::SpecialNot
+ } else {
+ UnaryOperator::PGPostfixFactorial
+ };
Review Comment:
I think we can simplify this match block a couple ways:
- We can remove the `match expr` block above that returns the `parser_err`
- We can add a `self.dialect.supports_bang_not_operator()` this would be
true for hive
then for this logic we do
```rust
let op = if self.dialect.supports_factorial_operator() {
UnaryOperator::PGPostfixFactorial
} else if self.dialect.supports_bang_not_operator() {
UnaryOperator::SpecialNot
} else {
self.expected("...)
}
```
##########
tests/sqlparser_hive.rs:
##########
@@ -537,6 +537,33 @@ fn parse_use() {
);
}
+#[test]
+fn parse_hive_unary_ops() {
+ let hive_unary_ops = &[("!", UnaryOperator::SpecialNot)];
+
+ for (str_op, op) in hive_unary_ops {
Review Comment:
the single element for loop seems superflous, we can write the SQL query
directly?
##########
tests/sqlparser_hive.rs:
##########
@@ -537,6 +537,33 @@ fn parse_use() {
);
}
+#[test]
+fn parse_hive_unary_ops() {
Review Comment:
one general comment regarding the tests, since we're relying on the dialect
api to guard the functionality, can we move the tests to common.rs and write
them in this style?
https://github.com/apache/datafusion-sqlparser-rs/blob/main/tests/sqlparser_common.rs#L2781
Also we should do the same for the factorial tests since the current PR
seems to be introducing two APIs to the dialect trait
##########
src/ast/operator.rs:
##########
@@ -51,6 +51,8 @@ pub enum UnaryOperator {
PGPrefixFactorial,
/// Absolute value, e.g. `@ -9` (PostgreSQL-specific)
PGAbs,
+ /// Special Not, e.g. `! false` (Hive-specific)
+ SpecialNot,
Review Comment:
```suggestion
/// Unary logical not operator: e.g. `! false` (Hive-specific)
BangNot,
```
Figured we can say Bang instead of Special since its conventional to call
bang operator
##########
src/dialect/mod.rs:
##########
@@ -561,6 +561,11 @@ pub trait Dialect: Debug + Any {
fn supports_asc_desc_in_column_definition(&self) -> bool {
false
}
+
+ /// Returns true if the dialect supports `a!` expressions
+ fn supports_factorial_operator(&self) -> bool {
+ false
+ }
Review Comment:
Should this be true instead? Looking at the current behavior,
`PGPostfixFactorial` functionality doesn't seem to be restricted to the
postgres dialect so that there's potential for a change in behavior if other
dialects currently rely on the behavior
##########
src/parser/mod.rs:
##########
@@ -1174,6 +1175,14 @@ impl<'a> Parser<'a> {
),
})
}
+ Token::ExclamationMark if
!self.dialect.supports_factorial_operator() => {
Review Comment:
```suggestion
Token::ExclamationMark if
self.dialect.supports_bang_not_operator() => {
```
See comment a bit lower for rationale to have a dedicated api for this
behavior
--
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
