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

[SPARK-35778][SQL][TESTS] Check multiply/divide of year month interval of any fields by numeric #33051

Closed
wants to merge 1 commit into from

Conversation

Peng-Lei
Copy link
Contributor

@Peng-Lei Peng-Lei commented Jun 24, 2021

What changes were proposed in this pull request?

Check multiply/divide of year-month intervals of any fields by numeric.

Why are the changes needed?

To improve test coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Expanded existed test cases.

@github-actions github-actions bot added the SQL label Jun 24, 2021
@Peng-Lei Peng-Lei changed the title Add test for multi and divide of year month interval of any fields [SPARK-35778][SQL][TEST] Add test for multi and divide of year month interval of any fields Jun 24, 2021
@Peng-Lei Peng-Lei changed the title [SPARK-35778][SQL][TEST] Add test for multi and divide of year month interval of any fields [SPARK-35778][SQL][TESTS] Check multiply/divide of year month interval of any fields Jun 24, 2021
@AmplabJenkins
Copy link

Can one of the admins verify this patch?

@Peng-Lei Peng-Lei changed the title [SPARK-35778][SQL][TESTS] Check multiply/divide of year month interval of any fields [SPARK-35778][SQL][TEST] Add test for multi and divide of year month interval of any fields by numeric Jun 24, 2021
@Peng-Lei Peng-Lei changed the title [SPARK-35778][SQL][TEST] Add test for multi and divide of year month interval of any fields by numeric [SPARK-35778][SQL][TESTS] Check multiply/divide of year month interval of any fields by numeric Jun 24, 2021
Copy link
Member

@MaxGekk MaxGekk left a comment

Choose a reason for hiding this comment

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

+1, LGTM. Merging to master.
Thank you, @Peng-Lei .

@MaxGekk MaxGekk closed this in 3904c0e Jun 24, 2021
@AngersZhuuuu
Copy link
Contributor

@MaxGekk this pr is wrong according to #33056 ...

@Peng-Lei
Copy link
Contributor Author

@MaxGekk this pr is wrong according to #33056 ...

@MaxGekk this pr is wrong according to #33056 ...

I also think when the same Period convert to different year month interval should be different value to store. So I also think the PR is wrong. we should wait the another PR,then rewite this PR change.

@MaxGekk
Copy link
Member

MaxGekk commented Jun 24, 2021

@AngersZhuuuu @Peng-Lei How about this #33014 . Is it wrong too?

@AngersZhuuuu
Copy link
Contributor

@AngersZhuuuu @Peng-Lei How about this #33014 . Is it wrong too?

Yes, In #33056 I have ignore these two pr's change, and after merge #33056, we will raise follow up to revert current wrong change and add correct UT.

@MaxGekk
Copy link
Member

MaxGekk commented Jun 24, 2021

Let me revert both #33014 and #33051.

@MaxGekk
Copy link
Member

MaxGekk commented Jun 24, 2021

@Peng-Lei Next time, let's add checks for year-month and day-time interval types in one PR.

@Peng-Lei
Copy link
Contributor Author

@Peng-Lei Next time, let's add checks for year-month and day-time interval types in one PR.

@MaxGekk OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants