Postgres: enhance NUMERIC/DECIMAL parsing to support negative scale

[IndexSeek]

Closes #1923

Beginning in PostgreSQL 15, it is allowed to declare a numeric column with a negative scale.

Source: PostgreSQL Documentation - Numeric Types

Changes

• Modified ExactNumberInfo::PrecisionAndScale to use i64 instead of u64 for scale
• Added parse_scale_value() method to handle positive, negative, and explicitly positive scale values
• Updated tests to cover negative scale scenarios

[iffyio]

I think we might be able to simplify this to something like the following?

if !self.consume_token(Token::Minus) {
    return i64::try_from(self.parse_literal_uint()?)
}

let next_token = self.next_token_ref();
match &next_token.token {
  Token::Number(s, _) => Self::parse::<i64>(s, next_token.span.start),
  _ => self.expected_ref("literal int", next_token),
}

[IndexSeek]

Wow, yes, this is much simpler and a way better way to handle the signs.

Thank you!

[iffyio]

I think the negative test cases can be merged into test_ansii_exact_numeric_types? since they're the same feature. Also would let us avoid the duplicate tests between the two functions

[IndexSeek]

I agree, this is a bit redundant.

[IndexSeek]

Thank you so much for the detailed review @iffyio! I think I have addressed your comments. Please let me know if there are any other changes I should make.

[iffyio]

was the pattern in the previous comment example insufficient? I think the current approach to optionally multiply by negative makes things slightly longer to follow what the code is trying to achieve.

fyi note ideally we use next_token_ref since that avoid cloning, especially given that parse::<i64> does not require an owned value

[IndexSeek]

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

    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.

[iffyio]

Ah yeah that makes sense, I confused peek_token_ref and was hoping to avoid the clone entirely but looking at Self::parse now I see its not currently set up to take in references so the current approach to clone the string sounds reasonable! Here's a couple minor updates to your example I think should do what we intended?

    fn parse_signed_integer(&mut self) -> Result<i64, ParserError> {
        if self.consume_token(&Token::Minus) {
            return i64::try_from(self.parse_literal_uint()?)
                .map(|v| -v)
                .or_else(|_| self.expected_ref("i64 literal", self.peek_token_ref()))
        }

        let _ = self.consume_token(&Token::Plus);
        self.advance_token();
        let next_token = self.get_current_token();
        match &next_token.token {
            Token::Number(s, _) => Self::parse::<i64>(s.clone(), next_token.span.start),
            _ => self.expected_ref("literal int", next_token),
        }
    }

[IndexSeek]

Thanks for the suggestion! It does read cleaner not starting with negation, looking back, opening with if !self.consume_token(&Token::Minus) felt a bit strange.

I like how your solution still handles the + sign, but I noticed a few issues with the logic flow. I think that with an optional + token on let _ = self.consume_token(&Token::Plus); followed by self.advance_token() and then self.get_current_token() could advance twice and potentially skip the actual number token.

I did take a look at peek_token_ref and was wondering if we could use it like so?

    fn parse_signed_integer(&mut self) -> Result<i64, ParserError> {
        let is_negative = self.consume_token(&Token::Minus);

        if !is_negative {
            let _ = self.consume_token(&Token::Plus);
        }

        let current_token = self.peek_token_ref();
        match &current_token.token {
            Token::Number(s, _) => {
                let s = s.clone();
                let span_start = current_token.span.start;
                self.advance_token();
                let value = Self::parse::<i64>(s, span_start)?;
                Ok(if is_negative { -value } else { value })
            }
            _ => self.expected_ref("number", current_token),
        }
    }

I think that should avoid us needing to consume the token until we know we have a number.

[IndexSeek]

Should we still parse the "+" character or error out? I know I originally added a test for it but I'm not sure any dialects support a "+" token in the scale definition for numeric types.