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

For testing GA #34

Closed
wants to merge 1 commit into from
Closed

Conversation

MaxGekk
Copy link

@MaxGekk MaxGekk commented Apr 8, 2021

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

@github-actions
Copy link

github-actions bot commented Apr 8, 2021

Test build #728916852 for PR 34 at commit 5c184b5.

@github-actions github-actions bot added the DOCS label Apr 8, 2021
@HyukjinKwon HyukjinKwon closed this Apr 9, 2021
HyukjinKwon added a commit to apache/spark that referenced this pull request Apr 14, 2021
…sitories to share the resources

### What changes were proposed in this pull request?

This PR proposes to leverage the GitHub Actions resources from the forked repositories instead of using the resources in ASF organisation at GitHub.

This is how it works:

1. "Build and test" (`build_and_test.yml`)  triggers a build on any commit on any branch (except `branch-*.*`), which roughly means:
    - The original repository will trigger the build on any commits in `master` branch
    - The forked repository will trigger the build on any commit in any branch.
2. The build triggered in the forked repository will checkout the original repository's `master` branch locally, and merge the branch from the forked repository into the original repository's `master` branch locally.
  Therefore, the tests in the forked repository will run after being sync'ed with the original repository's `master` branch.
3. In the original repository, it triggers a workflow that detects the workflow triggered in the forked repository, and add a comment, to the PR, pointing out the workflow in forked repository.

In short, please see this example HyukjinKwon#34

1. You create a PR and your repository triggers the workflow. Your PR uses the resources allocated to you for testing.
2. Apache Spark repository finds your workflow, and links it in a comment in your PR

**NOTE** that we will still run the tests in the original repository for each commit pushed to `master` branch. This distributes the workflows only in PRs.

### Why are the changes needed?

ASF shares the resources across all the ASF projects, which makes the development slow down.
Please see also:
- Discussion in the buildsa.o mailing list: https://lists.apache.org/x/thread.html/r48d079eeff292254db22705c8ef8618f87ff7adc68d56c4e5d0b4105%3Cbuilds.apache.org%3E
- Infra ticket: https://issues.apache.org/jira/browse/INFRA-21646

By distributing the workflows to use author's resources, we can get around this issue.

### Does this PR introduce _any_ user-facing change?

No, this is a dev-only change.

### How was this patch tested?

Manually tested at HyukjinKwon#34 and HyukjinKwon#33.

Closes #32092 from HyukjinKwon/poc-fork-resources.

Lead-authored-by: HyukjinKwon <[email protected]>
Co-authored-by: Hyukjin Kwon <[email protected]>
Signed-off-by: HyukjinKwon <[email protected]>
HyukjinKwon pushed a commit that referenced this pull request Oct 13, 2021
…on to make Java 17 compatible with Java 8

### What changes were proposed in this pull request?
The `date_format` function with `B` format has different behavior when use Java 8 and Java 17, `select date_format('2018-11-17 13:33:33.333', 'B')` in `datetime-formatting-invalid.sql` can prove this.

The case result with Java 8 is

```
-- !query
select date_format('2018-11-17 13:33:33.333', 'B')
-- !query schema
struct<>
-- !query output
java.lang.IllegalArgumentException
Unknown pattern letter: B
```

and the case result with Java 17 is

```
- datetime-formatting-invalid.sql *** FAILED ***
  datetime-formatting-invalid.sql
  Expected "struct<[]>", but got "struct<[date_format(2018-11-17 13:33:33.333, B):string]>" Schema did not match for query #34
  select date_format('2018-11-17 13:33:33.333', 'B'): -- !query
  select date_format('2018-11-17 13:33:33.333', 'B')
  -- !query schema
  struct<date_format(2018-11-17 13:33:33.333, B):string>
  -- !query output
  in the afternoon (SQLQueryTestSuite.scala:469)
```

We found that this is due to the new support of format `B` in Java 17

```
'B' is used to represent Pattern letters to output a day period in Java 17

     *  Pattern  Count  Equivalent builder methods
     *  -------  -----  --------------------------
     *    B       1      appendDayPeriodText(TextStyle.SHORT)
     *    BBBB    4      appendDayPeriodText(TextStyle.FULL)
     *    BBBBB   5      appendDayPeriodText(TextStyle.NARROW)
```

And through [ http://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html]( http://spark.apache.org/docs/latest/sql-ref-datetime-pattern.html) , we can confirm that format `B` is not documented/supported for `date_format` function currently.

So the main change of this pr is manual disabled format `B` of `date_format` function in `DateTimeFormatterHelper` to make Java 17 compatible with Java 8.

### Why are the changes needed?
Ensure that Java 17 and Java 8 have the same behavior.

### Does this PR introduce _any_ user-facing change?
No.

### How was this patch tested?

- Pass the Jenkins or GitHub Action
- Manual test `SQLQueryTestSuite` with JDK 17

**Before**

```
- datetime-formatting-invalid.sql *** FAILED ***
  datetime-formatting-invalid.sql
  Expected "struct<[]>", but got "struct<[date_format(2018-11-17 13:33:33.333, B):string]>" Schema did not match for query #34
  select date_format('2018-11-17 13:33:33.333', 'B'): -- !query
  select date_format('2018-11-17 13:33:33.333', 'B')
  -- !query schema
  struct<date_format(2018-11-17 13:33:33.333, B):string>
  -- !query output
  in the afternoon (SQLQueryTestSuite.scala:469)
```

**After**
The test `select date_format('2018-11-17 13:33:33.333', 'B')` in `datetime-formatting-invalid.sql`  passed

Closes apache#34237 from LuciferYang/SPARK-36970.

Authored-by: yangjie01 <[email protected]>
Signed-off-by: Max Gekk <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants