jacobcbeaudin commented on PR #63470:
URL: https://github.com/apache/airflow/pull/63470#issuecomment-5497570053
Thank you for your patience. I did a rebase onto the latest `main`. The
conflicts are gone. The two `Merge branch 'main'` commits are also gone. The
branch is now a clean rebase.
## Test results
@jroachgolf84 asked:
> One other thing; were you able to test this E2E in Snowflake? Does this
work as expected?
I ran this operator against a live Snowflake account. These functions are
correct:
- Parameters go to the notebook unchanged. `["param1", "target_db=PROD"]`
becomes `sys.argv == ['param1', 'target_db=PROD']`.
- A notebook that fails makes the task fail. The log shows the Python error
from Snowflake, not a general error.
- A notebook name that does not exist gives an immediate error: `SQL
compilation error: Notebook '...' does not exist`. The task does not wait.
- The operator sends one statement for each run. `QUERY_HISTORY` shows two
rows, but Snowflake tags the second row `StreamlitEngine`. That row is the
internal notebook query from Snowflake.
- The operator escapes backslashes correctly.
## The template question
@jroachgolf84 asked:
> Hmmm, I don't think that `self.notebook` or `self.parameters` are "going
to change" after templating... What would cause these to change?
Airflow causes the change. Airflow writes the rendered values back into the
template fields. You can do this test without a Snowflake account. The field
has the name `notebook_parameters` now, but the behaviour is the same:
```python
op = SnowflakeNotebookOperator(task_id="t", notebook="{{ params.db }}.NB",
notebook_parameters=["{{ params.name }}",
"plain"])
op.render_template_fields({"params": {"db": "MY_DB.S", "name": "O'Brien"}})
# op.notebook_parameters is now ["O'Brien", "plain"] -- Airflow wrote it in
place
# op.sql (old value): EXECUTE NOTEBOOK MY_DB.S.NB('O'Brien', 'plain') <-
not valid SQL
# execute() makes: EXECUTE NOTEBOOK MY_DB.S.NB('O''Brien', 'plain') <-
correct
```
Airflow writes the rendered values into the template fields in place. Thus
`execute()` must build the SQL again. Without the rebuild, the operator escapes
the Jinja text, and the rendered value goes to Snowflake unescaped. I added the
test `test_real_template_rendering_escapes_rendered_value`.
## A different name for the parameters
The tests showed me that `parameters` is a bad name here.
`SQLExecuteQueryOperator` uses the same name for SQL bind parameters.
The parent class can read bind parameters from a `.sql` or `.json` file.
This is an intended function of the parent class. `parameters="params.json"`
goes to `prepare_template()`, then to `ast.literal_eval`, and becomes a
dictionary.
But that function is wrong for notebook arguments. The two names were the
same. Thus Airflow found a notebook parameter with the form of a file name.
Airflow then replaced that parameter with the contents of the file:
```
notebook_parameters=["config.json", "plain"]
# after resolve_template_files(): ['{"secret": "..."}', 'plain']
```
I did these three things:
- The name is now `notebook_parameters`.
- `template_ext` is now `()`. This operator builds its own SQL and never
reads SQL from a file. Thus no template extension applies.
- `template_fields_renderers` keeps `{"sql": "sql"}`, the same as
`TeradataOperator`, `ExasolOperator` and `DatabricksSqlOperator`.
The new name also removes the `self.parameters = ...` line after
`super().__init__()`. The name conflict made that line necessary.
The new name agrees with the other operators in this hierarchy:
`session_parameters` and `client_parameters`.
Airflow did not release this operator yet. Thus the new name costs nothing
now. After a merge, a different name makes a deprecation cycle necessary.
Tell me if you prefer a smaller diff, and I will remove the new name.
--
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]
