Skip to content

Support IN UNNEST(expression) #426

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.

Sign up for GitHub

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

Jump to bottom
Merged
alamb merged 2 commits into from
Mar 1, 2022
Merged

Support IN UNNEST(expression) #426

alamb merged 2 commits into from
Mar 1, 2022

Conversation

@komukomo
Copy link
Contributor

@komukomo komukomo commented Feb 20, 2022

This PR adds support IN UNNEST(array_expression) for BigQuery.

search_value [NOT] IN value_set

value_set:
  {
    (expression[, ...])
    | (subquery)
    | UNNEST(array_expression)
  }

https://cloud.google.com/bigquery/docs/reference/standard-sql/operators#in_operators

@coveralls
Copy link

coveralls commented Feb 28, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 1913841843

  • 22 of 27 (81.48%) changed or added relevant lines in 3 files are covered.
  • 324 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.03%) to 90.548%
Changes Missing Coverage Covered Lines Changed/Added Lines %
src/parser.rs 7 8 87.5%
src/ast/mod.rs 5 9 55.56%
Files with Coverage Reduction New Missed Lines %
src/tokenizer.rs 1 90.44%
src/ast/query.rs 25 86.57%
src/ast/mod.rs 127 77.52%
src/parser.rs 171 84.27%
Totals Coverage Status
Change from base Build 1858728297: -0.03%
Covered Lines: 7175
Relevant Lines: 7924
💛 - Coveralls

alamb
alamb approved these changes Feb 28, 2022
View reviewed changes
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.

I apologize for the late review, I have been away for the last week. Thank you for the contribution @komukomo

src/ast/mod.rs
subquery: Box<Query>,
negated: bool,
},
/// `[ NOT ] IN UNNEST(array_expression)`
Copy link
Contributor

@alamb alamb Feb 28, 2022

Choose a reason for hiding this comment

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

I was going to suggest making this somewhat more general with any expr (rather than specific for UNEST function) like:

/// [NOT] IN <expr>
InExpr {
  expr: Box<Expr>,
  in_expr: Box<Expr>,
  negated: bool
}

However, I see that https://cloud.google.com/bigquery/docs/reference/standard-sql/operators#in_operators treats it as a special case so I think it would be ok to do here as well.

Actually, looking more deeply, it seems like UNNEST actually supports with offset as well

https://cloud.google.com/bigquery/docs/reference/standard-sql/query-syntax#unnest_operator

Copy link
Contributor Author

@komukomo komukomo Mar 1, 2022

Choose a reason for hiding this comment

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

Thank you for your valuable review.

As you said, it seems to be treated as a special case and any other functions can't used, so I did like this.

Actually, looking more deeply, it seems like UNNEST actually supports with offset as well

I did not know about with offset, thank you.
However, WITH OFFSET seems to be available when UNNEST returns a table.
In the case of IN UNNSET(array_expression), WITH OFFSET cannot be used because this IN UNNEST(array_expression) is a syntax sugar of subquery:
IN (SELECT element FROM UNNEST(array_expression) AS element).

✅ SELECT * FROM UNNEST(ARRAY[1,2,3]) WITH OFFSET
✅ SELECT 1 IN UNNEST(ARRAY[1,2,3])
❌ SELECT 1 IN UNNEST(ARRAY[1,2,3]) WITH OFFSET

So, if we support with offset, I think it is better to separate it from this PR, because the implementation and testing will be completely different. (I will try it in the future, if possible).

Copy link
Contributor

@alamb alamb Mar 1, 2022

Choose a reason for hiding this comment

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

So, if we support with offset, I think it is better to separate it from this PR,

I agree -- that makes sense. If anyone needs WITH OFFSET support it can be added as a follow on 👍

tests/sqlparser_common.rs
}

#[test]
fn parse_in_unnest() {
Copy link
Contributor

@alamb alamb Feb 28, 2022

Choose a reason for hiding this comment

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

Can you please also add a test for NOT IN UNNEST as well?

Copy link
Contributor Author

@komukomo komukomo Mar 1, 2022

Choose a reason for hiding this comment

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

Thanks! I will fix it!

@komukomo
Copy link
Contributor Author

komukomo commented Mar 1, 2022 •
edited
Loading

@alamb I fixed it!

@alamb
Copy link
Contributor

alamb commented Mar 1, 2022

Thanks again @komukomo

@alamb alamb merged commit 0d1c5d1 into apache:main Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alamb alamb alamb approved these changes

@houqp houqp houqp approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@komukomo @coveralls @alamb @houqp