Conversation
jsoref
left a comment
There was a problem hiding this comment.
Most corrections were automatically suggested by Google Sheets.
The doubling fixes were by me in a slightly more targeted manner, but I did try to skip the @param cases. I'll drop the ones I've flagged here.
I'm happy to drop anything as directed/split/etc.
|
|
||
| Each pull request submission will be reviewed by a contributor or [committer](https://github.com/prestodb/presto/wiki/committers) | ||
| in the project. Only committers may merge a pull request. Large contributions should have an associated Github issue. | ||
| in the project. Only committers may merge a pull request. Large contributions should have an associated GitHub issue. |
There was a problem hiding this comment.
This is a brand. (It's flagged by recently added tooling.)
| The Presto Web UI is composed of several React components and is written in JSX and ES6. This source code is compiled and packaged into browser-compatible Javascript, which is then checked in to the Presto source code (in the `dist` folder). You must have [Node.js](https://nodejs.org/en/download/) and [Yarn](https://yarnpkg.com/en/) installed to execute these commands. To update this folder after making changes, simply run: | ||
| The Presto Web UI is composed of several React components and is written in JSX and ES6. This source code is compiled and packaged into browser-compatible JavaScript, which is then checked in to the Presto source code (in the `dist` folder). You must have [Node.js](https://nodejs.org/en/download/) and [Yarn](https://yarnpkg.com/en/) installed to execute these commands. To update this folder after making changes, simply run: | ||
|
|
||
| yarn --cwd presto-main/src/main/resources/webapp/src install | ||
|
|
||
| If no Javascript dependencies have changed (i.e., no changes to `package.json`), it is faster to run: | ||
| If no JavaScript dependencies have changed (i.e., no changes to `package.json`), it is faster to run: |
| fi | ||
|
|
||
| # For Migwn, ensure paths are in UNIX format before anything is touched | ||
| # For Mingw, ensure paths are in UNIX format before anything is touched |
There was a problem hiding this comment.
Also a brand (but, not flagged by the same tooling, this was just caught by the fact that it isn't in the dictionary).
| * Presto module to do all kinds of run Guice injection stuff! | ||
| * <p> | ||
| * WARNING: Contains black magick | ||
| * WARNING: Contains black magic |
There was a problem hiding this comment.
Some projects are intentionally cute, this one doesn't appear to be particularly.
presto-accumulo/src/main/java/com/facebook/presto/accumulo/index/IndexLookup.java
Outdated
Show resolved
Hide resolved
| .add("preGroupedSymbols", preGroupedSymbols) | ||
| .add("masks", masks) | ||
| .add("groudId", groupId) | ||
| .add("groupId", groupId) |
presto-mongodb/src/main/java/com/facebook/presto/mongodb/WriteConcernType.java
Outdated
Show resolved
Hide resolved
presto-product-tests/src/main/resources/com/facebook/presto/tests/cli/batch_query.results
Outdated
Show resolved
Hide resolved
| * </ol> | ||
| * <p> This prioritizes query performance over total cluster storage capacity, and therefore may | ||
| * produce a cluster state that is imbalanced in terms of disk utilization. | ||
| * produce a cluster state that is uneven in terms of disk utilization. |
| // Until we migrate all connectors to parametrized varchar we check two options | ||
| assertTrue(actual.equals(expectedParametrizedVarchar) || actual.equals(expectedUnparametrizedVarchar), | ||
| format("%s does not matche neither of %s and %s", actual, expectedParametrizedVarchar, expectedUnparametrizedVarchar)); | ||
| format("%s matches neither %s nor %s", actual, expectedParametrizedVarchar, expectedUnparametrizedVarchar)); |
b8ddb61 to
6ff65c7
Compare
rschlussel
left a comment
There was a problem hiding this comment.
This is awesome. Can you also update the commit messages to say" Fix Spelling: ..." instead of just "Spelling ..." to match our commit guidelines https://github.com/prestodb/presto/wiki/Review-and-Commit-guidelines#commit-formatting-and-pull-requests.
presto-main/src/test/java/com/facebook/presto/operator/aggregation/TestHistogram.java
Outdated
Show resolved
Hide resolved
presto-main/src/test/java/com/facebook/presto/sql/planner/TestRowExpressionFormatter.java
Outdated
Show resolved
Hide resolved
presto-accumulo/src/main/java/com/facebook/presto/accumulo/index/IndexLookup.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
curious about the changes to add "example". Is that to avoid accidentally creating a real uri?
There was a problem hiding this comment.
Yes, if you don't own a domain you shouldn't use it. example is allowed per DNS RFCs.
doesnotmatter.com is a real domain backed by real web servers and people shouldn't compose URLs that might go to things they don't control.
There are a number of examples of various major vendors (and individuals) messing up the internet in scary ways because they included strings and didn't realize that users might actually use the things. (A number of the examples are from Microsoft, but plenty aren't.)
| } | ||
|
|
||
| If you want to allow users to use the extractly same name as their Kerberos principal | ||
| If you want to allow users to use exactly same name as their Kerberos principal |
There was a problem hiding this comment.
should be "exactly the same name" (add the)
| public static final ConnectorTableMetadata METADATA = tableMetadataBuilder(NAME) | ||
| .column("procedure_cat", createUnboundedVarcharType()) | ||
| .column("procedure_schem", createUnboundedVarcharType()) | ||
| .column("procedure_schema", createUnboundedVarcharType()) |
There was a problem hiding this comment.
Can you pull this change out? it needs a release note since it's a column name in a table we expose. I also want to double check whether there's anything else we need to do to make this change.
presto-main/src/main/java/com/facebook/presto/connector/system/jdbc/ProcedureJdbcTable.java
Outdated
Show resolved
Hide resolved
presto-mongodb/src/main/java/com/facebook/presto/mongodb/WriteConcernType.java
Outdated
Show resolved
Hide resolved
...metastore/src/main/java/com/facebook/presto/hive/metastore/alluxio/AlluxioHiveMetastore.java
Outdated
Show resolved
Hide resolved
presto-benchto-benchmarks/src/main/resources/sql/presto/tpcds/q44.sql
Outdated
Show resolved
Hide resolved
presto-benchto-benchmarks/src/main/resources/sql/presto/tpcds/q63.sql
Outdated
Show resolved
Hide resolved
| public Iterable<RowExpression> nonInferrableConjuncts(RowExpression expression) | ||
| public Iterable<RowExpression> nonInferableConjuncts(RowExpression expression) | ||
| { | ||
| return filter(extractConjuncts(expression), not(isInferenceCandidate())); | ||
| } | ||
|
|
||
| public static Iterable<RowExpression> nonInferrableConjuncts(Metadata metadata, RowExpression expression) | ||
| public static Iterable<RowExpression> nonInferableConjuncts(Metadata metadata, RowExpression expression) |
There was a problem hiding this comment.
thanks. this is not an api
Includes some minor grammatical fixes as well. Notable change: `JOURNAL_SAFEY` -> `JOURNAL_SAFE` Signed-off-by: Josh Soref <jsoref@users.noreply.github.com>
Test plan
This PR corrects misspellings identified by the check-spelling action.
The misspellings have been reported at jsoref@f42e814#commitcomment-67876501
The action reports that the changes in this PR would make it happy: jsoref@4a11b58
Note: this PR does not include the action. If you're interested in running a spell check on every PR and push, that can be offered separately.
Please make sure your submission complies with our Development, Formatting, and Commit Message guidelines. Don't forget to follow our attribution guidelines for any code copied from other projects.
Fill in the release notes towards the bottom of the PR description.
See Release Notes Guidelines for details.