Skip to content
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

feat: support current_schema and session_user #4358

Merged
merged 9 commits into from
Aug 3, 2022
Merged

feat: support current_schema and session_user #4358

merged 9 commits into from
Aug 3, 2022

Conversation

neverchanje
Copy link
Contributor

@neverchanje neverchanje commented Aug 2, 2022

I hereby agree to the terms of the Singularity Data, Inc. Contributor License Agreement.

What's changed and what's your intention?

PLEASE DO NOT LEAVE THIS EMPTY !!!

Support two functions that DBeaver uses.

Checklist

  • I have written necessary rustdoc comments
  • I have added necessary unit tests and integration tests
  • All checks passed in ./risedev check (or alias, ./risedev c)

Documentation

If your pull request contains user-facing changes, please specify the types of the changes, and create a release note. Otherwise, please feel free to remove this section.

Types of user-facing changes

  • SQL commands, functions, and operators
  1. current_schema | current_schema(): Returns the current schema. This is the schema that will be used for any tables or other named objects that are created without specifying a target schema.
  2. session_user : Returns the session user's name.

Release note

Please create a release note for your changes. In the release note, focus on the impact on users, and mention the environment or conditions where the impact may occur.

Refer to a related PR or issue link (optional)

@neverchanje neverchanje added the user-facing-changes Contains changes that are visible to users label Aug 2, 2022
@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

Merging #4358 (e3cdb64) into main (38173fc) will increase coverage by 0.01%.
The diff coverage is 90.00%.

@@            Coverage Diff             @@
##             main    #4358      +/-   ##
==========================================
+ Coverage   74.55%   74.56%   +0.01%     
==========================================
  Files         847      847              
  Lines      123720   123716       -4     
==========================================
+ Hits        92242    92253      +11     
+ Misses      31478    31463      -15     
Flag Coverage Δ
rust 74.56% <90.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/frontend/src/handler/dml.rs 0.00% <0.00%> (ø)
src/frontend/src/handler/explain.rs 0.00% <0.00%> (ø)
src/frontend/src/handler/query.rs 0.00% <0.00%> (ø)
src/frontend/src/session.rs 41.72% <0.00%> (+0.98%) ⬆️
src/frontend/src/binder/expr/function.rs 80.59% <100.00%> (+0.44%) ⬆️
src/frontend/src/binder/expr/mod.rs 76.20% <100.00%> (+0.36%) ⬆️
src/frontend/src/binder/expr/value.rs 92.43% <100.00%> (ø)
src/frontend/src/binder/mod.rs 100.00% <100.00%> (ø)
src/frontend/src/binder/values.rs 98.27% <100.00%> (ø)
src/frontend/src/handler/create_mv.rs 97.04% <100.00%> (-0.06%) ⬇️
... and 6 more

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

@neverchanje neverchanje requested review from xiangjinwu and st1page and removed request for xiangjinwu August 3, 2022 06:59
@@ -41,7 +41,19 @@ impl Binder {
}
Expr::Row(exprs) => self.bind_row(exprs),
// input ref
Expr::Identifier(ident) => self.bind_column(&[ident]),
Expr::Identifier(ident) => {
if ["session_user"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Also current_schema?

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 would like to unsupport it until we really need to use it :) Postgres has so many features that nobody knows whether it will be used one day.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just want to add a second example so this .iter().any() reads less confusing. I was wondering why not just "session_user" == ident.xxx until I realized it can extend to support other "parentheses-less functions".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, let me fix it 😄

Comment on lines +253 to +254
#[tokio::test]
async fn test_bind_value() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is async tokio test preferred?

Copy link
Contributor Author

@neverchanje neverchanje Aug 3, 2022

Choose a reason for hiding this comment

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

It's because we use SessionImpl::mock and somehow it uses an async function internally.

    #[cfg(test)]
    pub fn mock_binder() -> Binder {
-        mock_binder_with_catalog(Catalog::default(), "".to_string())
+        Binder::new(&SessionImpl::mock())
    }

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting ... It is a runtime error rather than compile time.

@mergify mergify bot merged commit c4183cd into main Aug 3, 2022
@mergify mergify bot deleted the wt-time branch August 3, 2022 09:43
nasnoisaac pushed a commit to nasnoisaac/risingwave that referenced this pull request Aug 9, 2022
* feat: support current_schema and session_user

* fix

* fix test

* fix clippy

* remove session user test

* add second system var

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/feature user-facing-changes Contains changes that are visible to users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants