IndexSeek commented on code in PR #1990:
URL: 
https://github.com/apache/datafusion-sqlparser-rs/pull/1990#discussion_r2252750662


##########
src/parser/mod.rs:
##########
@@ -11229,6 +11229,30 @@ impl<'a> Parser<'a> {
         }
     }
 
+    /// Parse an optionally signed integer literal.
+    fn parse_signed_integer(&mut self) -> Result<i64, ParserError> {
+        let next_token = self.next_token();
+        let (sign, number_token) = match next_token.token {
+            Token::Minus => {
+                let number_token = self.next_token();
+                (-1, number_token)
+            }
+            Token::Plus => {
+                let number_token = self.next_token();
+                (1, number_token)
+            }
+            _ => (1, next_token),
+        };

Review Comment:
   When I tried to implement your suggested pattern, it failed on test cases 
with explicit `+` signs (like `NUMERIC(10,+5)`) because `parse_literal_uint()` 
expected a number token. I should have posted my issue earlier. 
   
   I actually went back and tested that PostgreSQL doesn't even take that 
(ERROR: type modifiers must be simple constants or identifiers), so I was 
overcomplicating it and I will remove this test case.
   
   > fyi note ideally we use next_token_ref since that avoid cloning, 
especially given that parse::<i64> does not require an owned value
   
   I did not find `next_token_ref` but it looks that `advance_token()` + 
`get_current_token()` will give us this behavior (#1618)? 
   
   Here's what I am working with right now after removing that explicit "+" 
test:
   
   ```rs
       fn parse_signed_integer(&mut self) -> Result<i64, ParserError> {
           if !self.consume_token(&Token::Minus) {
               return i64::try_from(self.parse_literal_uint()?)
                   .map_err(|_| ParserError::ParserError("Integer 
overflow".to_string()));
           }
   
           self.advance_token();
           let next_token = self.get_current_token();
           match &next_token.token {
               Token::Number(s, _) => {
                   let positive_value = Self::parse::<i64>(s.clone(), 
next_token.span.start)?;
                   Ok(-positive_value)
               }
               _ => self.expected_ref("literal int", next_token),
           }
       }
   ```
   
   I believe this should only clone the string data and not the tokens now.



-- 
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

Reply via email to