Skip to content

[SPARK-31183][SQL][FOLLOWUP] Move rebase tests to AvroSuite and check the rebase flag out of function bodies #27964

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
Closed
MaxGekk wants to merge 2 commits into from
Closed

[SPARK-31183][SQL][FOLLOWUP] Move rebase tests to AvroSuite and check the rebase flag out of function bodies #27964

MaxGekk wants to merge 2 commits into from

Conversation

@MaxGekk
Copy link
Member

@MaxGekk MaxGekk commented Mar 20, 2020

What changes were proposed in this pull request?

  1. The tests added by [SPARK-31183][SQL] Rebase date/timestamp from/to Julian calendar in Avro #27953 are moved from AvroLogicalTypeSuite to AvroSuite.
  2. Checking of the rebaseDateTime flag is moved out from functions bodies.

Why are the changes needed?

  1. The tests are moved because they are not directly related to logical types.
  2. Checking the flag out of functions bodies should improve performance.

Does this PR introduce any user-facing change?

No

How was this patch tested?

By running Avro tests via the command build/sbt avro/test

@MaxGekk
Copy link
Member Author

MaxGekk commented Mar 20, 2020

@HyukjinKwon @cloud-fan Please, review the PR.

@SparkQA
Copy link

SparkQA commented Mar 20, 2020

Test build #120085 has finished for PR 27964 at commit 556d438.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Mar 20, 2020

Merged to master and branch-3.0.

HyukjinKwon pushed a commit that referenced this pull request Mar 20, 2020
@MaxGekk @HyukjinKwon
[SPARK-31183][SQL][FOLLOWUP] Move rebase tests to AvroSuite and che…
a6f3e3b 
…ck the rebase flag out of function bodies

1. The tests added by #27953 are moved from `AvroLogicalTypeSuite` to `AvroSuite`.
2. Checking of the `rebaseDateTime` flag is moved out from functions bodies.

1. The tests are moved because they are not directly related to logical types.
2. Checking the flag out of functions bodies should improve performance.

No

By running Avro tests via the command `build/sbt avro/test`

Closes #27964 from MaxGekk/rebase-avro-datetime-followup.

Authored-by: Maxim Gekk <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 22, 2020 •
edited
Loading

Hi, All.
This seems to break all eight Jenkins jobs on branch-3.0 consistently.

org.apache.spark.sql.avro.AvroV1LogicalTypeSuite.Logical type: user specified output schema with different timestamp types
org.apache.spark.sql.avro.AvroV1Suite.SPARK-31183: rebasing milliseconds timestamps in write
org.apache.spark.sql.avro.AvroV2LogicalTypeSuite.Logical type: user specified output schema with different timestamp types
org.apache.spark.sql.avro.AvroV2Suite.SPARK-31183: rebasing milliseconds timestamps in write

@dongjoon-hyun
Copy link
Member

dongjoon-hyun commented Mar 22, 2020

The answers are different. It seems that we need more date-time related patches at branch-3.0. I'm not sure what it is.

!== Correct Answer - 2 ==                        == Spark Answer - 2 ==
!struct<>                                        struct<timestamp_millis:timestamp,timestamp_micros:timestamp>
![1969-12-31 16:00:01.0,1969-12-31 16:00:02.0]   [1969-12-31 16:00:01.0,1970-01-23 19:33:20.0]
![1969-12-31 16:11:06.0,1969-12-31 16:16:39.0]   [1969-12-31 16:11:06.0,2001-08-28 05:00:00.0]

Given the above, I'll try to recover branch-3.0 first by reverting this. Please make a separate PR on branch-3.0 to see the result.

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Mar 22, 2020

Thanks @dongjoon-hyun.

@MaxGekk
Copy link
Member Author

MaxGekk commented Mar 22, 2020

The problem is commits of this PR in master and in branch-3.0 are different:
master:
DateTimeUtils.microsToMillis at b402bc9#diff-01fea32e6ec6bcf6f34d06282e08705aR155

branch-3.0:
DateTimeUtils.fromMillis at a6f3e3b#diff-01fea32e6ec6bcf6f34d06282e08705aR155

The fromMillis() in branch-3.0 is millisToMicros() but not microsToMillis() actually. The renaming was done in master by #27618

@MaxGekk MaxGekk mentioned this pull request Mar 22, 2020
Closed
@MaxGekk
Copy link
Member Author

MaxGekk commented Mar 22, 2020

Here is the backport of this PR with bug fix: #27977

@HyukjinKwon
Copy link
Member

HyukjinKwon commented Mar 22, 2020

Ah, sorry, seems it was my mistake. Thanks @MaxGekk

HyukjinKwon pushed a commit that referenced this pull request Mar 23, 2020
@MaxGekk @HyukjinKwon
[SPARK-31183][SQL][FOLLOWUP][3.0] Move rebase tests to AvroSuite an…
4bc4af3 
…d check the rebase flag out of function bodies

### What changes were proposed in this pull request?
1. The tests added by #27953 are moved from `AvroLogicalTypeSuite` to `AvroSuite`.
2. Checking of the `rebaseDateTime` flag is moved out from functions bodies.

This is a backport of #27964

### Why are the changes needed?
1. The tests are moved because they are not directly related to logical types.
2. Checking the flag out of functions bodies should improve performance.

### Does this PR introduce any user-facing change?
No

### How was this patch tested?
By running Avro tests via the command `build/sbt avro/test`

Closes #27977 from MaxGekk/rebase-avro-datetime-followup-3.0.

Authored-by: Maxim Gekk <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
@MaxGekk MaxGekk mentioned this pull request Apr 14, 2020
Closed
sjincho pushed a commit to sjincho/spark that referenced this pull request Apr 15, 2020
@MaxGekk
[SPARK-31183][SQL][FOLLOWUP] Move rebase tests to AvroSuite and che…
23e7ba5 
…ck the rebase flag out of function bodies

### What changes were proposed in this pull request?
1. The tests added by apache#27953 are moved from `AvroLogicalTypeSuite` to `AvroSuite`.
2. Checking of the `rebaseDateTime` flag is moved out from functions bodies.

### Why are the changes needed?
1. The tests are moved because they are not directly related to logical types.
2. Checking the flag out of functions bodies should improve performance.

### Does this PR introduce any user-facing change?
No

### How was this patch tested?
By running Avro tests via the command `build/sbt avro/test`

Closes apache#27964 from MaxGekk/rebase-avro-datetime-followup.

Authored-by: Maxim Gekk <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
@MaxGekk MaxGekk deleted the rebase-avro-datetime-followup branch June 5, 2020 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cloud-fan cloud-fan cloud-fan approved these changes

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@MaxGekk @SparkQA @HyukjinKwon @dongjoon-hyun @cloud-fan