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.

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

feat(expr): Support UInt32, UInt64, Int32, Int64 in new expression #6660

Merged
merged 3 commits into from
Jul 18, 2022
Merged

feat(expr): Support UInt32, UInt64, Int32, Int64 in new expression #6660

merged 3 commits into from
Jul 18, 2022

Conversation

jiaoew1991
Copy link
Contributor

@jiaoew1991 jiaoew1991 commented Jul 16, 2022 •
edited
Loading

Signed-off-by: Enwei Jiao jiaoew2011@gmail.com

I hereby agree to the terms of the CLA available at: https://databend.rs/dev/policies/cla/

Summary

Support more Int family types in new expression

Fixes #6636

@vercel
Copy link

vercel bot commented Jul 16, 2022 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
databend ✅ Ready (Inspect) Visit Preview Jul 18, 2022 at 10:55AM (UTC)

@mergify
Copy link
Contributor

mergify bot commented Jul 16, 2022

This pull request's title is not fulfill the requirements. @jiaoew1991 please update it 🙏.

Valid format:

fix(query): fix group by string bug
  ^         ^---------------------^
  |         |
  |         +-> Summary in present tense.
  |
  +-------> Type: feat, fix, refactor, ci, build, docs, website, chore

Valid types:

  • feat: this PR introduces a new feature to the codebase
  • fix: this PR patches a bug in codebase
  • refactor: this PR changes the code base without new features or bugfix
  • ci|build: this PR changes build/testing/ci steps
  • docs|website: this PR changes the documents or websites
  • chore: this PR only has small changes that no need to record

@jiaoew1991 jiaoew1991 changed the title Support UInt32, UInt64, Int32, Int64 in new expression feat(expr): Support UInt32, UInt64, Int32, Int64 in new expression Jul 16, 2022
@mergify mergify bot added the pr-feature this PR introduces a new feature to the codebase label Jul 16, 2022
@xudong963 xudong963 requested a review from andylokandy July 16, 2022 15:14
andylokandy
andylokandy approved these changes Jul 17, 2022
View reviewed changes
Copy link
Contributor

@andylokandy andylokandy left a comment

Choose a reason for hiding this comment

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

LGTM

common/expression/src/types/number.rs Show resolved Hide resolved
common/expression/src/types/number.rs Show resolved Hide resolved
@andylokandy
Copy link
Contributor

andylokandy commented Jul 17, 2022 •
edited
Loading

#6661 has introduced some conflicts. There are two major changes that will affect this PR:

  • run_ast test helper function now takes an expression string and parses it into RawExpr, which is good for ergonomics, but because the common_ast parser doesn't recognize negative numbers (-10 is recognized as UnaryOp::Minus(Literal::Integer(10))), you may add an unary overload for minus to construct negative number from positive number.
  • Evaluator::run_cast now returns Result, which means we can throw runtime error for downgrade casts now. With this change, we may allow any casting between numbers in can_cast_to and then throw a runtime error when we do find a number is too large or too small.

@andylokandy andylokandy mentioned this pull request Jul 17, 2022
Merged
@jiaoew1991
Support UInt32, UInt64, Int32, Int64 in new expression
ba6b9b8 
Signed-off-by: Enwei Jiao <jiaoew2011@gmail.com>
@jiaoew1991
fix common_super_type when type equal but sign not equal
90c51a7 
Signed-off-by: Enwei Jiao <jiaoew2011@gmail.com>
@jiaoew1991
Copy link
Contributor Author

@andylokandy any more review suggestions?

@Xuanwo
Copy link
Member

Xuanwo commented Jul 18, 2022 •
edited
Loading

This PR looks well good now! Let's require another LGTM from @sundy-li @ygf11 @xudong963

@andylokandy
Copy link
Contributor

LGTM now

@ygf11
Copy link
Contributor

ygf11 commented Jul 18, 2022

LGTM

@mergify mergify bot merged commit 110a510 into databendlabs:main Jul 18, 2022
@jiaoew1991 jiaoew1991 deleted the 6636-int-family branch July 20, 2022 02:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andylokandy andylokandy andylokandy approved these changes

@ygf11 ygf11 ygf11 left review comments

@sundy-li sundy-li sundy-li left review comments

@leiysky leiysky leiysky approved these changes

Assignees
No one assigned
Labels
pr-feature this PR introduces a new feature to the codebase
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support UInt32, UInt64, Int32, Int64, Float32, Float64 in new expression framework
6 participants
@jiaoew1991 @andylokandy @Xuanwo @ygf11 @sundy-li @leiysky