Skip to content

Add support for first, last aggregate function parsing #880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

mustafasrepo
Copy link
Contributor

@mustafasrepo mustafasrepo commented May 11, 2023

Closes #877

This PR adds support for FIRST, LAST aggregate functions.
As an example, see queries below

SELECT FIRST(x ORDER BY x) AS a FROM T

or

SELECT LAST(x ORDER BY x) AS a FROM T

With this PR we can parse above queries successfully.
Common dialects support this feature, for instance, duckdb (see link), databricks (see link) supports this feature.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 4947357822

  • 46 of 78 (58.97%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.1%) to 86.045%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/ast/mod.rs 18 22 81.82%
src/parser.rs 24 52 46.15%
Totals Coverage Status
Change from base Build 4931911477: -0.1%
Covered Lines: 14342
Relevant Lines: 16668

💛 - Coveralls

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @mustafasrepo -- sorry for the delay in reviewing.

@@ -3549,6 +3555,66 @@ impl fmt::Display for ArrayAgg {
}
}

/// An `FIRST` invocation `FIRST( [ DISTINCT ] <expr> [ORDER BY <expr>] [LIMIT <n>] )`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these new structs follow the existing patterns in this file but they seem very specific to me (as in do we really need two new structs that are almost identical to each other as well as to Function.

Did you consider adding an order_by clause to Function and adjusting the parse_function function?

So something like

pub struct Function {
    pub name: ObjectName,
    pub args: Vec<FunctionArg>,
    pub over: Option<WindowSpec>,
    // aggregate functions may specify eg `COUNT(DISTINCT x)`
    pub distinct: bool,
    // Some functions must be called without trailing parentheses, for example Postgres
    // do it for current_catalog, current_schema, etc. This flags is used for formatting.
    pub special: bool,
    // optional ORDER BY expression <----------- Proposed Addition
    pub order_by: Option<Box<OrderByExpr>>,
}

I think this would both reduce the code required for this PR and make the codebase easier to maintain, as well as would extend naturally to other aggregate functions (e.g. databricks offers the first_value function in addition to first)

Adding an order_by to Function would allow users to parse any function with an order_by clause which I think would be quite valuable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't consider, this. However, it seems like a better approach to me. I will experiment with it, then let you know about result. Thanks for the suggestion.

self.expect_token(&Token::LParen)?;
let expr = Box::new(self.parse_expr()?);
// ANSI SQL and BigQuery define ORDER BY inside function.
if !self.dialect.supports_within_after_array_aggregation() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand the call to supports_within_after_array_aggregation here as this is not an array aggregate.


for sql in [
"SELECT FIRST(x ORDER BY x) AS a FROM T",
"SELECT LAST(x ORDER BY x) AS a FROM T",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given there is code above to handle WITHIN GROUP during parsing, can you either: 1) Remove that code, or 2) add test coverage for it?

        // Snowflake defines ORDERY BY in within group instead of inside the function like
        // ANSI SQL.
        self.expect_token(&Token::RParen)?;
        let within_group = if self.parse_keywords(&[Keyword::WITHIN, Keyword::GROUP]) {
            self.expect_token(&Token::LParen)?;
            self.expect_keywords(&[Keyword::ORDER, Keyword::BY])?;
            let order_by_expr = self.parse_order_by_expr()?;
            self.expect_token(&Token::RParen)?;
            Some(Box::new(order_by_expr))
        } else {
            None
        };

@mustafasrepo mustafasrepo marked this pull request as draft May 18, 2023 12:41
@mustafasrepo
Copy link
Contributor Author

@alamb. I have tried you suggestion. New PR puts order by field inside Function. I think new version is much more clear. Until we decide for final approach, both PRs will open (I have converted this to draft). By the way, I didn't do some of the reviews in this PR, (since they are obsolete in the new PR). If we somehow, decide to continue with this approach, I will address them. You can find new Pr in the link

@alamb
Copy link
Contributor

alamb commented May 18, 2023

I agree #882 looks great and I have merged that one in

@mustafasrepo mustafasrepo deleted the feature/first_last_support branch May 22, 2023 06:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for FIRST, LAST aggregation parsing.
3 participants