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

fix(expr): bool has different output format and cast-string format #3233

Merged
merged 3 commits into from
Jun 15, 2022

Conversation

xiangjinwu
Copy link
Contributor

@xiangjinwu xiangjinwu commented Jun 15, 2022

What's changed and what's your intention?

Fixes #3146 by introducing a dedicated BoolOut expr. All other types can share the same expr/func for I/O and from/to-string cast.

Check doc comment of cast_output for details. The 2 different implementations in PostgreSQL are called boolout and booltext.

In PostgreSQL, only 4 types have cast-to-string different from output format: bool, cidr, inet, xml, and only 1 type has cast-from-string different from input format: xml.

Checklist

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

Refer to a related PR or issue link (optional)

@github-actions github-actions bot added the type/fix Bug fix label Jun 15, 2022
@codecov
Copy link

codecov bot commented Jun 15, 2022

Codecov Report

Merging #3233 (2989f77) into main (e87390a) will increase coverage by 0.01%.
The diff coverage is 52.94%.

❗ Current head 2989f77 differs from pull request most recent head 65c121d. Consider uploading reports for the commit 65c121d to get more accurate results

@@            Coverage Diff             @@
##             main    #3233      +/-   ##
==========================================
+ Coverage   73.42%   73.43%   +0.01%     
==========================================
  Files         744      744              
  Lines      101658   101673      +15     
==========================================
+ Hits        74639    74663      +24     
+ Misses      27019    27010       -9     
Flag Coverage Δ
rust 73.43% <52.94%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
src/expr/src/expr/expr_unary.rs 70.98% <0.00%> (-1.12%) ⬇️
src/expr/src/vector_op/cast.rs 70.06% <0.00%> (-1.46%) ⬇️
src/expr/src/expr/mod.rs 51.02% <100.00%> (ø)
src/frontend/src/expr/function_call.rs 96.49% <100.00%> (+2.92%) ⬆️
src/frontend/src/expr/mod.rs 86.97% <100.00%> (+0.37%) ⬆️
src/frontend/src/expr/type_inference.rs 98.97% <100.00%> (+<0.01%) ⬆️
src/meta/src/barrier/mod.rs 69.13% <0.00%> (-0.33%) ⬇️
src/storage/src/hummock/local_version_manager.rs 84.01% <0.00%> (+0.15%) ⬆️
src/meta/src/hummock/mock_hummock_meta_client.rs 43.95% <0.00%> (+1.09%) ⬆️
... and 1 more

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

@xiangjinwu xiangjinwu marked this pull request as ready for review June 15, 2022 06:05
if self.return_type() == DataType::Boolean {
return Ok(FunctionCall::new(ExprType::BoolOut, vec![self])?.into());
}
self.cast_assign(DataType::Varchar)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we use cast_assign instead of cast_explicit here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given the target is hard-coded to Varchar, both assign and explicit would allow the cast. There is no difference, as the error checking inside always pass.

@xiangjinwu xiangjinwu enabled auto-merge (squash) June 15, 2022 07:16
@xiangjinwu xiangjinwu merged commit 2f9d522 into main Jun 15, 2022
@xiangjinwu xiangjinwu deleted the distinguish-bool-to-string branch June 15, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

boolean in concat should be formatted to t f instead of true false
2 participants