sarutak commented on PR #59425: URL: https://github.com/apache/spark/pull/59425#issuecomment-6141234106
Also, a few small things on the description before merge, since it lands in git log as the commit message: 1. Accuracy: the first "Why are the changes needed?" bullet says the blocklist "checks for `Command` subclasses (`CreateTableAsSelect`, `InsertIntoStatement`, etc.)". Those two examples are actually the plans that are NOT `Command` subclasses: `CreateTableAsSelect extends V2CreateTableAsSelectPlan` and `InsertIntoStatement extends UnaryParsedStatement`, and `blockUnsupportedSqlCommand` checks them separately for exactly that reason (its own comment reads "Following commands are not subclasses of `Command` but have side effects"). Suggest rewording to distinguish the two, for example: "The blocklist rejects any `Command`, plus a few side-effecting plans that are not `Command` subclasses (`CreateTableAsSelect`, `InsertIntoStatement`, etc.)". 2. Minor mechanism accuracy (optional): the same bullet says commands inside reference `SubqueryAlias` inputs execute "because `eagerlyExecuteCommands` walks the resolved plan tree". For a reference input the command actually runs because `executeSQLWithRefs` eagerly executes each reference as its own query via `Dataset.ofRows(session, transformRelation(ref.input))` to register the temp view. `eagerlyExecuteCommands` is the tree-walk mechanism for a command nested within a single query plan, which is a different path. The conclusion (reference commands run as a side effect) is correct; only the stated mechanism is slightly off. 3. Template headings: the section headings use `##` but the Spark PR template uses `###` (`### What changes were proposed in this pull request?`, etc.). Minor, but worth matching the template. 4. Could you drop the opening line "This replaces #56241, which was closed as stale without a review. It is rebased on the current master with no other changes."? That context is useful in the PR conversation, but it becomes noise in the permanent commit message. The PR link history already records the lineage. -- 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]
