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

[MINOR][DOCS] Fix SQL Error links and link anchors #44825

Closed
wants to merge 1 commit into from

Conversation

nchammas
Copy link
Contributor

What changes were proposed in this pull request?

Link anchors are case sensitive (at least on Safari). Many of the links in the SQL error pages use the incorrect case, so the anchor doesn't jump to the correct heading. This PR fixes those anchors.

There is also a bad link in the menu, which this PR also fixes.

Why are the changes needed?

Links should point to their intended target.

Does this PR introduce any user-facing change?

Yes, user-facing documentation.

How was this patch tested?

I built the SQL docs with:

SKIP_SCALADOC=1 SKIP_PYTHONDOC=1 SKIP_RDOC=1 bundle exec jekyll build

And I clicked around a bunch of the SQL error pages to confirm the link anchors now work correctly.

I also confirmed the menu link also works correctly now.

Was this patch authored or co-authored using generative AI tooling?

No.

@github-actions github-actions bot added the DOCS label Jan 22, 2024
@nchammas
Copy link
Contributor Author

cc @itholic

@HyukjinKwon
Copy link
Member

Merged to master.

@itholic
Copy link
Contributor

itholic commented Jan 23, 2024

Late LGTM. Thanks for the fix 🙂

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

@nchammas . This PR seems to break the unit tests.

@dongjoon-hyun
Copy link
Member

@nchammas
Copy link
Contributor Author

Oh, interesting. How did the build pass here?

dongjoon-hyun added a commit that referenced this pull request Jan 23, 2024
### What changes were proposed in this pull request?

This PR regenerated the documents with the following.
```
SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "core/testOnly *SparkThrowableSuite -- -t \"Error classes match with document\""
```

### Why are the changes needed?

The following PR broke CIs by manually fixing the generated docs.

- #44825

Currently, CI is broken like the following.
- https://github.com/apache/spark/actions/runs/7619269448/job/20752056653
- https://github.com/apache/spark/actions/runs/7619199659/job/20751858197

```
[info] - Error classes match with document *** FAILED *** (24 milliseconds)
[info]   "...lstates.html#class-0[A]-feature-not-support..." did not equal "...lstates.html#class-0[a]-feature-not-support..." The error class document is not up to date. Please regenerate it. (SparkThrowableSuite.scala:322)
```

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

No.

### How was this patch tested?

Manually check.
```
$ build/sbt "core/testOnly *.SparkThrowableSuite"
...
[info] SparkThrowableSuite:
[info] - No duplicate error classes (31 milliseconds)
[info] - Error classes are correctly formatted (47 milliseconds)
[info] - SQLSTATE is mandatory (2 milliseconds)
[info] - SQLSTATE invariants (26 milliseconds)
[info] - Message invariants (8 milliseconds)
[info] - Message format invariants (7 milliseconds)
[info] - Error classes match with document (65 milliseconds)
[info] - Round trip (28 milliseconds)
[info] - Error class names should contain only capital letters, numbers and underscores (7 milliseconds)
[info] - Check if error class is missing (15 milliseconds)
[info] - Check if message parameters match message format (4 milliseconds)
[info] - Error message is formatted (1 millisecond)
[info] - Error message does not do substitution on values (1 millisecond)
[info] - Try catching legacy SparkError (0 milliseconds)
[info] - Try catching SparkError with error class (1 millisecond)
[info] - Try catching internal SparkError (0 milliseconds)
[info] - Get message in the specified format (6 milliseconds)
[info] - overwrite error classes (61 milliseconds)
[info] - prohibit dots in error class names (23 milliseconds)
[info] Run completed in 1 second, 357 milliseconds.
[info] Total number of tests run: 19
[info] Suites: completed 1, aborted 0
[info] Tests: succeeded 19, failed 0, canceled 0, ignored 0, pending 0
[info] All tests passed.
```

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #44843 from dongjoon-hyun/SPARK-46804.

Authored-by: Dongjoon Hyun <dhyun@apple.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
@dongjoon-hyun
Copy link
Member

PR builder has a short-circuit for doc-only PR. So, core module test is not triggered.

@nchammas nchammas deleted the sql-error-links branch January 23, 2024 02:22
HyukjinKwon pushed a commit that referenced this pull request Jan 23, 2024
…s docs

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

Add an HTML comment to all generated SQL error class documents warning the reader that they should not edit the file.

### Why are the changes needed?

This should help prevent problems like the one in #44825.

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

No.

### How was this patch tested?

I ran the following command and reviewed the modified documents:

```sh
SPARK_GENERATE_GOLDEN_FILES=1 build/sbt "core/testOnly *SparkThrowableSuite -- -t \"Error classes match with document\""
```

### Was this patch authored or co-authored using generative AI tooling?

No.

Closes #44847 from nchammas/SPARK-46807-sql-error-automation-notice.

Authored-by: Nicholas Chammas <nicholas.chammas@gmail.com>
Signed-off-by: Hyukjin Kwon <gurwls223@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants