peter-toth commented on code in PR #57724:
URL: https://github.com/apache/spark/pull/57724#discussion_r3713205160
##########
docs/sql-ref-syntax-dml-merge-into.md:
##########
@@ -69,6 +71,13 @@ not_matched_by_source_action
The source of the merge, specified either as a table or as a parenthesized
query. An optional
alias may be provided with or without the `AS` keyword.
+* **WITH ( option_key = option_value [ , ... ] )**
+
+ Specifies dynamic table options for this `MERGE` operation. Options
following the target table
+ are passed to the data source connector for the write operation. Options
following the source
+ table are passed to the connector when reading the source. The supported
options depend on the
+ connector.
Review Comment:
**Finding 2.** "for the write operation" undersells the target options.
`RewriteMergeIntoTable.scala:130` passes the target relation's options into
`buildOperationTable(tbl, MERGE, r.options)`, which wraps them in
`RowLevelOperationInfoImpl` and hands them to the connector's
`newRowLevelOperationBuilder(info)`. The `RowLevelOperation` that comes back
receives those options on **both** sides --
`RowLevelOperation.newScanBuilder(CaseInsensitiveStringMap options)`
(`sql/catalyst/src/main/java/org/apache/spark/sql/connector/write/RowLevelOperation.java:86`)
as well as `newWriteBuilder` -- and
`RewriteRowLevelCommand.buildRelationWithAttrs` (used at
`RewriteMergeIntoTable.scala:162`) keeps them on the target scan relation too.
That matters for the likeliest use of a target option: a copy-on-write
`MERGE` tuning the *read* of the affected groups would be told by this page
that target options only reach the write.
```suggestion
Specifies dynamic table options for this `MERGE` operation. Options
following the target table
are passed to the data source connector for the row-level operation on
the target, which uses
them both when reading the affected rows and when writing the result.
Options following the
source table are passed to the connector when reading the source. The
supported options depend
on the connector.
```
##########
docs/sql-ref-syntax-dml-insert-table.md:
##########
@@ -46,6 +50,11 @@ INSERT [ WITH SCHEMA EVOLUTION ] INTO [ TABLE ]
table_identifier [ BY NAME ] REP
**Syntax:** `[ database_name. ] table_name`
+* **WITH ( option_key = option_value [ , ... ] )**
+
+ Specifies dynamic table options for this `INSERT` operation. These options
are passed to the
+ data source connector when writing to the table. The supported options
depend on the connector.
Review Comment:
**Finding 4.** Every example this PR adds uses a backticked key -- `` WITH
(`write.split-size` = 10) `` here, the same on the `MERGE` page, `` WITH
(`split-size` = 5) `` on `SELECT` -- and none of the three bullets says why. It
is not cosmetic: `propertyKey` is `errorCapturingIdentifier (DOT
errorCapturingIdentifier)*` (`SqlBaseParser.g4:691-693`), so the `-` makes a
bare `write.split-size` unparseable and a reader who drops the backticks gets a
syntax error with nothing on the page to explain it. `propertyKeyOrStringLit`
also accepts a string literal, so `'write.split-size' = 10` works as well.
The `UPDATE` / `DELETE FROM` bullets in the sibling PR #57725 already state
this, which is a second reason to say it here -- otherwise the same clause is
documented with and without the rule depending on which page a reader lands on.
```suggestion
Specifies dynamic table options for this `INSERT` operation. These
options are passed to the
data source connector when writing to the table. Keys and values are
treated as strings; a key
that is not a valid identifier, such as `write.split-size`, must be
quoted with backticks or
written as a string literal. The supported options depend on the
connector.
```
Worth mirroring on `docs/sql-ref-syntax-dml-merge-into.md:76` and
`docs/sql-ref-syntax-qry-select.md:107` too.
##########
docs/sql-ref-syntax-qry-select.md:
##########
@@ -191,6 +202,16 @@ SELECT [ hints , ... ] [ ALL | DISTINCT ] { [ [
named_expression | regex_column_
Specifies a hive-style transform query specification to transform the
input by forking and running user-specified command or script.
+### Examples
+
+#### Select Using Dynamic Table Options
Review Comment:
**Finding 6.** `students` is not defined anywhere on this page -- it belongs
to `docs/sql-ref-syntax-dml-insert-table.md`. Since this is the page's first
`### Examples` section there is no earlier setup to lean on, and the sibling
pages spell the assumption out: `docs/sql-ref-syntax-dml-merge-into.md:138-139`
("The following examples assume that the `target` and `source` tables have
already been created and populated..."), and both new pages in #57725 open
their Examples the same way. One line covers it.
```suggestion
### Examples
The following example assumes that a `students` table has already been
created and populated.
#### Select Using Dynamic Table Options
```
##########
docs/sql-ref-syntax-dml-merge-into.md:
##########
@@ -33,7 +33,9 @@ apply. All of these row-level changes are performed as a
single atomic operation
```sql
MERGE [ WITH SCHEMA EVOLUTION ] INTO target_table [ [ AS ] target_alias ]
- USING { source_table | ( source_query ) } [ [ AS ] source_alias ]
+ [ WITH ( target_option_key = target_option_value [ , ... ] ) ]
+ USING { source_table [ WITH ( source_option_key = source_option_value [ ,
... ] ) ] |
Review Comment:
**Finding 5.** These four placeholders have no entry in Parameters: the
bullet added at `:74` is titled `WITH ( option_key = option_value [ , ... ] )`,
so a reader looking up `target_option_key` finds nothing. Every other
placeholder in this syntax block (`target_table`, `source_table`,
`merge_condition`, ...) has a matching bullet. The bullet body already
distinguishes target from source, so the shorter names in the syntax block are
enough -- and they then match the `INSERT` and `SELECT` pages.
```suggestion
[ WITH ( option_key = option_value [ , ... ] ) ]
USING { source_table [ WITH ( option_key = option_value [ , ... ] ) ] |
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
