Skip to content

Conversation

@panbingkun
Copy link
Contributor

@panbingkun panbingkun commented Jul 23, 2024

What changes were proposed in this pull request?

The pr aims to improve the docs related to variable, includes:

  • docs/sql-ref-syntax-aux-set-var.md, show the primitive error messages.
  • docs/sql-ref-syntax-ddl-declare-variable.md, add usage of DECLARE OR REPLACE.
  • docs/sql-ref-syntax-ddl-drop-variable.md, show the primitive error messages and fix typo.

Why are the changes needed?

Only improve docs.

Does this PR introduce any user-facing change?

Yes, make end-user docs clearer.

How was this patch tested?

Manually test.

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

No.

@github-actions github-actions bot added the DOCS label Jul 23, 2024
@panbingkun
Copy link
Contributor Author

panbingkun commented Jul 23, 2024

Additionally, the syntax seems a bit weird. When setting variables, we support (VARIABLE | VAR), but when declaring and dropping variables, we only support the keyword VARIABLE

image

setStatementWithOptionalVarKeyword
: SET (VARIABLE | VAR)? assignmentList #setVariableWithOptionalKeyword
| SET (VARIABLE | VAR)? LEFT_PAREN multipartIdentifierList RIGHT_PAREN EQ
LEFT_PAREN query RIGHT_PAREN #setVariableWithOptionalKeyword
;

| DECLARE (OR REPLACE)? VARIABLE?
identifierReference dataType? variableDefaultExpression? #createVariable
| DROP TEMPORARY VARIABLE (IF EXISTS)? identifierReference #dropVariable

Should we unify it?

@panbingkun
Copy link
Contributor Author

cc @srielau @cloud-fan @HyukjinKwon

@panbingkun panbingkun marked this pull request as ready for review July 23, 2024 09:15
If you did not qualify the name with a schema and catalog, verify the current_schema() output, or qualify the name with the correct schema and catalog.
To tolerate the error on drop use DROP VARIABLE IF EXISTS. SQLSTATE: 42883

-- Drop temporart variable if it exists
Copy link
Contributor Author

Choose a reason for hiding this comment

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

typo
temporart -> temporary

Choose a different name, or drop or replace the existing variable. SQLSTATE: 42723

-- Use `DECLARE OR REPLACE` to declare a defined variable
DECLARE OR REPLACE five = 55;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The usage of DECLARE OR REPLACE does not exist in the doc

Copy link
Contributor

Choose a reason for hiding this comment

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

to double check, the doc does mention it but we don't show it in the example, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this doc mentions it in the Syntax section, but don't show it in the example.
image

Copy link
Contributor

@allisonwang-db allisonwang-db left a comment

Choose a reason for hiding this comment

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

@HyukjinKwon
Copy link
Member

Merged to master.

ilicmarkodb pushed a commit to ilicmarkodb/spark that referenced this pull request Jul 29, 2024
### What changes were proposed in this pull request?
The pr aims to improve the docs related to `variable`, includes:
- `docs/sql-ref-syntax-aux-set-var.md`, show the `primitive` error messages.
- `docs/sql-ref-syntax-ddl-declare-variable.md`, add usage of `DECLARE OR REPLACE`.
- `docs/sql-ref-syntax-ddl-drop-variable.md`, show the `primitive` error messages and fix `typo`.

### Why are the changes needed?
Only improve docs.

### Does this PR introduce _any_ user-facing change?
Yes, make end-user docs clearer.

### How was this patch tested?
Manually test.

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

Closes apache#47460 from panbingkun/SPARK-48976.

Authored-by: panbingkun <panbingkun@baidu.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

Development

Successfully merging this pull request may close these issues.

4 participants