Improved SQL rendering within BigQueryInsertJobOperator

[nathadfield]

Description

BigQueryInsertJobOperator requires the submission of a configuration parameter in the form of a dict. Unfortunately, if this contains a large SQL query - especially one that is formatted with new lines - then this cannot currently be rendered very nicely in the UI.

Use case / motivation

The issue with this is that it's impossible to copy and paste the rendered query out of the Airflow UI, into a BigQuery browser and run it without lots of manual edits which is time wasted when troubleshooting problems.

Are you willing to submit a PR?

Yes. My current thought process around this would be to add an optional SQL parameter to the operator which, if provided, would be added into the configuration and could therefore have its own template field and SQL renderer.

e.g.

[potiuk]

How about adding a general capability of having "view" on parameters that will not require copying?

Simply add possibility of specifying jsonpath (https://goessner.net/articles/JsonPath/) expression that could generate a virtual parameter to render. This could be done on the operator static fields rather than as parameter. We could take kwargs and convert them to - essentially - json, using some well defined serialization (maybe even current serializaton we use would do).

I imagine something like that at operator level:

renderable_values= [
   ('sql', '$.configuration.query.query', 'sql'),
]

where 'renderable values will be an array of tuples ("NAME", "JSONPATH", "RENDERER"),

That would be very natural extension of the current rendering approach and something that can be generalized and easy to add for a number of operators (and no need for optional parameters).

[nathadfield]

@potiuk Thanks for the input. When you talk about a general capability, where would this sit? I don't really understand yet the relationships between the operator parameters and the UI rendering.

[kaxil]

Wouldn't just using strip where you define the large SQL suffice?

❯ python
Python 3.8.5 (default, Sep  4 2020, 02:22:02)
[Clang 10.0.0 ] :: Anaconda, Inc. on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> str1 = "\n Starbucks has the best coffee \n"
>>> newstr = str1.strip()
>>> print(newstr)
Starbucks has the best coffee

>>> str1 = "\n Starbucks has the best \n hi coffee \n"
>>> newstr = str1.strip()
>>> print(newstr)
Starbucks has the best
 hi coffee
>>>

[nathadfield]

@kaxil It doesn't look like it.

sql = '''
    INSERT INTO `target_project.target_dataset.target_table`
    (col1, col2, col3, col4)
    SELECT col1, col2, col3, col4
    FROM `source_project.source_dataset.source_table`
    '''

    test = BigQueryInsertJobOperator(
        task_id='test',
        gcp_conn_id='my_gcp_connection',
        configuration={"query": {"query": sql.strip(),
                                 "useLegacySql": False}
                       }
    )

The new lines characters before INSERT and after the trailing '`' have been removed but everything in between has not.

Regardless, I still wouldn't want a large SQL query with formatting to aid readability to be reduced to a single line character string. That wouldn't be helpful.

[tswast]

A UI improvement seems like the right choice. #17032 (comment)

There are a ton of properties on job resource objects, so I'd be worried about the precedent for adding kwargs for each of them. Especially since query jobs aren't the only kind of job

[potiuk]

Yeah. I think it's a nice feature to have it in this generic way. It is very much like multi-language rendering in modern IDEs :). @nathadfield are you up to the task :) ?

[potiuk]

Happy to guide on that one :)

[nathadfield]

@potiuk I would love to say yes, but I'm definitely lacking some knowledge. So far I've figured out that there would need to be a change to BaseOperator to enable the provision of a renderable_values object on an operator.

I also can see that there would need to be some additional logic in views.py but this is about as far as I've managed to get so far.

Any guidance or tips you can provide would be most appreciated.

[kaxil]

Looks like usage of triple quotes adds line breaks -- https://stackoverflow.com/questions/19756715/remove-n-from-triple-quoted-string-in-python explains it better.

I think the DAG Author should handle this as for other instances a line break is on purpose and not just used for multi-line strings.

For the feature rendering, Jarek proposed, let's try to gauge the benefits of that feature first. Because any changes in serialization will increase the size of the payload to push during parsing in the scheduler and for the webserver to pull.

This can get rid of newline chars if you don't need

    sql = '''
    INSERT INTO `target_project.target_dataset.target_table` \
    (col1, col2, col3, col4) \
    SELECT col1, col2, col3, col4 \
    FROM `source_project.source_dataset.source_table`
    '''

You can remove tabs and others, by using dedent.

But Airflow shows what you pass, trying to guess what user is doing won't be accurate all the time in my opinion

[potiuk]

If that works for now, cool.

Just one question- the feature i proposed is UI-only really ( i believe ?) Why serialization would be impacted ? I think it's just one more class level field at the operator to tell UI to make a bit more work on json form of the operator that is already there.

But maybe I am wrong? Is it because it is class -level field and we need to handle it differently?

I do not know details of serialization that much and maybe I missed something. But I agree if it involves some changes in serialization protocol, it should not be taken lightly.

What i like about the idea however is that i see a lot of cases - especially in Google operators, where the parameters are complex structures and it would be nice to be able to format and see them with syntax coloring and copy-paste able . That's a nice feature.

[nathadfield]

@kaxil Thanks for that but, if I am being very nit-picky, it is a lot harder to copy a long query out of the UI if its spread over one line as it requires scrolling to the right without really knowing where it ends. It is a lot easier to capture the query if it has been rendered as SQL with the intended formatting.

The impact of this is that pasting it into a BigQuery browser results in a single line query.

In this example its not a big deal but if its a complicated query that needs troubleshooting then I either need to reformat it, or get the untemplated query and insert the correct variables to replicate the query that run. (FYI - The BigQuery browser formatter does not do a very good of this)

Both of these scenarios result in wasted time that we didn't have to worry about prior to 2.x BigQuery operators so I feel like there has been a bit of a regression here.

[kaxil]

Both of these scenarios result in wasted time that we didn't have to worry about prior to 2.x BigQuery operators so I feel like there has been a bit of a regression here.

Oh ! if that's a regression, yes we should definitely fix it. I wonder it might be due to serialization too. I will take a closer look at it, if someone does not fix it before me :)

Just one question- the feature i proposed is UI-only really ( i believe ?) Why serialization would be impacted ? I think it's just one more class level field at the operator to tell UI to make a bit more work on json form of the operator that is already there.

I must have mis-understood it, I thought you were proposing passing keys (or json schema path) in the operator. But yeah if UI only can work on the currently existing data and can use jmspath or something, definitely a +1

[nathadfield]

Both of these scenarios result in wasted time that we didn't have to worry about prior to 2.x BigQuery operators so I feel like there has been a bit of a regression here.

Oh ! if that's a regression, yes we should definitely fix it. I wonder it might be due to serialization too. I will take a closer look at it, if someone does not fix it before me :)

@kaxil When I say regression I'm more talking about from a UX perspective.

This operator didn't exist before v2 so we used BigQueryOperator which is now deprecated as is BigQueryExecuteQueryOperator in the provider in favour of BigQueryInsertJobOperator.

Using BigQueryExecuteQueryOperator gives the rendering we would like to see but at the expense of not having the full BigQuery job configuration available.

sql = '''
INSERT INTO `target_project.target_dataset.target_table`
(col1, col2, col3, col4)
SELECT col1, col2, col3, col4
FROM `source_project.source_dataset.source_table`
'''

test = BigQueryExecuteQueryOperator(
    task_id='test',
    gcp_conn_id='my_gcp_connection',
    sql=sql
)

[kaxil]

In that case what Jarek is proposing sounds reasonable to me as it is a dict