Skip to content

Tests for support try_cast/cast decimal to numeric - #1465

Merged
alamb merged 4 commits into
apache:masterfrom
liukun4515:cast_decimal_to_signed_numeric
Jan 16, 2022
Merged

Tests for support try_cast/cast decimal to numeric#1465
alamb merged 4 commits into
apache:masterfrom
liukun4515:cast_decimal_to_signed_numeric

Conversation

@liukun4515

@liukun4515liukun4515 commented Dec 18, 2021

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

part of #1443

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@liukun4515liukun4515 mentioned this pull request Dec 18, 2021
Closed
5 tasks
@liukun4515liukun4515 changed the title support decimal to numericsupport try_cast/cast decimal to numericDec 18, 2021
@liukun4515
liukun4515force-pushed the cast_decimal_to_signed_numeric branch from ec34918 to 3950f7eCompareDecember 19, 2021 09:23
@liukun4515
liukun4515force-pushed the cast_decimal_to_signed_numeric branch from 3950f7e to 89370f8CompareJanuary 13, 2022 03:44
@liukun4515
liukun4515 marked this pull request as ready for review January 13, 2022 03:44
@liukun4515

liukun4515 commented Jan 13, 2022•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

It's ready to review.
PTAL @alamb@houqp

@alamb

alamb commented Jan 13, 2022

Copy link
Copy Markdown
Contributor

I will try and review this later today but I may not be able to review until tomorow

@alambalamb changed the title support try_cast/cast decimal to numerictests fpr support try_cast/cast decimal to numericJan 14, 2022
@alambalamb changed the title tests fpr support try_cast/cast decimal to numericTests for support try_cast/cast decimal to numericJan 14, 2022
alamb
alamb approved these changes Jan 14, 2022
View reviewed changes

@alambalamb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks @liukun4515 -- the only comment I have is it would be nice to reduce some of the duplication if possible, but also a bit of duplication in the tests isn't a horrible thing either.

👍

Comment threaddatafusion/src/physical_plan/expressions/cast.rs
// 3. evaluate the expression
// 4. verify that the resulting expression is of type B
// 5. verify that the resulting values are downcastable and correct
macro_rules! generic_decimal_to_other_test_cast {

@alambalambJan 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

its unfortunate that this code shares so much with generic_test_cast (seems like everything except the creation of the array). I wonder if it would be possible to refactor the validation so that it could be shared.

@alamb

alamb commented Jan 14, 2022

Copy link
Copy Markdown
Contributor

It appears this PR has some conflicts that need to be resolved now

@liukun4515

liukun4515 commented Jan 16, 2022

Copy link
Copy Markdown
ContributorAuthor

Thanks @liukun4515 -- the only comment I have is it would be nice to reduce some of the duplication if possible, but also a bit of duplication in the tests isn't a horrible thing either.

👍

I have traced this as a subtask in #1443

@alamb
alamb merged commit 438b417 into apache:masterJan 16, 2022
@alamb

alamb commented Jan 16, 2022

Copy link
Copy Markdown
Contributor

Thanks again @liukun4515

@liukun4515
liukun4515 deleted the cast_decimal_to_signed_numeric branch January 16, 2022 15:00
unkloud pushed a commit to unkloud/datafusion that referenced this pull request Mar 23, 2025
@andygrove
build: Use stable channel in rust-toolchain (apache#1465)
569ca71
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alambalambalamb 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.

2 participants

@liukun4515@alamb