Skip to content

Fix a build issue in presto-docs#25178

Merged
steveburnett merged 1 commit intoprestodb:masterfrom
wraymo:doc_fix
May 23, 2025
Merged

Fix a build issue in presto-docs#25178
steveburnett merged 1 commit intoprestodb:masterfrom
wraymo:doc_fix

Conversation

@wraymo
Copy link
Contributor

@wraymo wraymo commented May 23, 2025

Description

This PR fixes a build issue in presto-docs

Motivation and Context

Previously, we got a build failure in presto-docs as described in #25177. This PR fixes it.

Impact

Test Plan

cd presto-docs
mvn install

Contributor checklist

  • Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

@rschlussel
Copy link
Contributor

why didn't this fail in the CI before?

Copy link
Contributor

@tdcmeehan tdcmeehan left a comment

Choose a reason for hiding this comment

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

Both should work, but I'm OK with this. Would be good to understand why it's a problem now.

Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

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

Lgtm, the build fails in my local without this fix.

It seems that the standard thrift syntax specification requires using commas(,) rather than semicolons(;) as separators between enum values. I guess both cases can build successfully may because the thrift compiler or toolchain used in local has implemented syntax compatibility handling.

@wraymo
Copy link
Contributor Author

wraymo commented May 23, 2025

Thanks for your review! The reason is there's an update of drift yesterday (v1.43). And for method renderEnumElements in ThriftIdlRenderer, the code changes builder.append(";\n"); to builder.append(",\n");, which requires us to use , as a separator.

@steveburnett
Copy link
Contributor

Rebasing and merging this now, based on the three approving reviews.

@steveburnett steveburnett merged commit bb77665 into prestodb:master May 23, 2025
102 checks passed
@steveburnett
Copy link
Contributor

steveburnett commented May 23, 2025

  1. Opened doc-only [docs] Fix formatting in sql/show-create-schema.rst #25188, docs / test (:presto-docs) (pull_request) failed.
  2. Merged this PR.
  3. Rebased and pushed branch for [docs] Fix formatting in sql/show-create-schema.rst #25188 to rerun the tests, docs / test (:presto-docs) (pull_request) was successful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants