Skip to content

Conversation

@huaxingao
Copy link
Contributor

What changes were proposed in this pull request?

clean up

  1. uncomment out result tables in SQL examples (to be consistent with other SQL examples)
  2. Fix small problems: as -> AS, select -> SELECT, statement -> statements
  3. Fix several syntax problems.

Why are the changes needed?

Fix errors in sql ref

Does this PR introduce any user-facing change?

No

How was this patch tested?

Manually build and check.

<li><code>*</code> alone matches 0 or more characters and <code>|</code> is used to separate multiple different regular expressions,
any of which can match. </li>
<li>The leading and trailing blanks are trimmed in the input pattern before processing. The pattern match is case-insensitive.</li>
</ul>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change to the same description of regex_pattern as the one used in SHOW TABLE EXTENDED, SHOW TABLES, SHOW VIEWS.

<li><code>*</code> alone matches 0 or more characters and <code>|</code> is used to separate multiple different regular expressions,
any of which can match. </li>
<li>The leading and trailing blanks are trimmed in the input pattern before processing. The pattern match is case-insensitive.</li>
</ul>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Change to the same description of regex_pattern as the one used in SHOW TABLE EXTENDED, SHOW TABLES, SHOW VIEWS.

-- org.apache.spark.sql.catalyst.analysis.HintErrorLogger: Hint (strategy=merge)
-- is overridden by another hint and will not take effect.
SELECT /*+ BROADCAST(t1) */ /*+ MERGE(t1, t2) */ * FROM t1 INNER JOIN t2 ON t1.key = t2.key;
SELECT /*+ BROADCAST(t1), MERGE(t1, t2) */ * FROM t1 INNER JOIN t2 ON t1.key = t2.key;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

SELECT /*+ BROADCAST(t1) */ /*+ MERGE(t1, t2) */ works too, but want to make the example consistent with the syntax /*+ join_hint [ , ... ] */

@huaxingao
Copy link
Contributor Author

cc @maropu

@SparkQA
Copy link

SparkQA commented May 1, 2020

Test build #122163 has finished for PR 28428 at commit abaf962.

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

{% highlight sql %}
-- This CREATE TABLE fails with ParseException because of the illegal identifier name a.b
CREATE TABLE test (a.b int);
-- output
Copy link
Member

Choose a reason for hiding this comment

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

This comment looks fine, but I think we need the same comment in the other error output.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe have the same format as ansi compliance page?
Screen Shot 2020-05-01 at 2 57 21 PM

Copy link
Member

Choose a reason for hiding this comment

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

I think adding -- output there looks okay, WDYT?

INSERT INTO t VALUES ('1');
-- output
  org.apache.spark.sql.AnalysisException: Cannot write incompatible data to table '`default`.`t`':
  - Cannot safely cast 'v': StringType to IntegerType;

Could you check documents in the other DBMS-like systems? As the others suggested, I think its better to follow the other document formats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I googled, but didn't have much luck. Only found this one at https://docs.snowflake.com/en/sql-reference/functions/validate.html

Screen Shot 2020-05-01 at 4 08 33 PM

I personally like to either put -- output or indent 2 spaces for the error message, or both (the way you suggested)

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@maropu maropu left a comment

Choose a reason for hiding this comment

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

Looks fine

@maropu
Copy link
Member

maropu commented May 1, 2020

btw, how many clean-up PRs are left?

@huaxingao
Copy link
Contributor Author

btw, how many clean-up PRs are left?

Two more:
remove all the html syntax following this PR #28414.
change HTML tables to markdown tables following this PR #28415 (@dilipbiswal might work on this)

@maropu
Copy link
Member

maropu commented May 1, 2020

change HTML tables to markdown tables following this PR #28415 (@dilipbiswal might work on this)

Do you have time to work on it, @dilipbiswal ?

@huaxingao
Copy link
Contributor Author

I will close this PR. I am changing every sql ref file to remove the html syntax. I will make these changes there.

@huaxingao huaxingao closed this May 3, 2020
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.

3 participants