SQL stored procs for labels to replace python procs #591

joshelser · 2024-03-20T01:39:51Z

Split up label tests into separate files

joshelser · 2024-03-20T01:45:06Z

Non-scientific results show create label latency on the low end around 500ms and on the upper end around 2.5s. I would imagine we can chop some of that off by consolidating the number of "validation rows". I wasn't focused on that in this pass (more focused on correctness and maintainable tests).

joshelser · 2024-03-21T14:52:19Z

Jacques mentioned that he wanted to take a look at this change, so I've tagged him for review.

joshelser · 2024-03-21T14:53:13Z

FWIW, there wasn't much in terms of python tests we added that weren't already encapsulated in the label test cases (largely, because we didn't write pytests that required Snowflake -- just exercising the pydantic validations).

jacques-n · 2024-03-21T16:32:11Z

Hey, can you separate this into two PRs?

PR1: Moving runtime validation logic from python to SQL.
PR2: Separating python tests into multiple files (with virtually no code changes).

jacques-n · 2024-03-21T16:37:14Z

bootstrap/004_labels.sql

+$$
+begin
+    -- Ungrouped labels
+    insert into internal.validate_labels (sql, message, simple, create_only) values


Have a few thoughts here.

Let's just make this a view

Let's use convention over configuration for optional arguments

Let's use Snowflake object literals to make this cleaner/clearer as opposed to positional

There is a bunch of duplicate logic. Let's factor out into common checks. Some are direct copy and paste. Some are simple modifications to avoid nearly complete duplication

I suggest we iterate on a couple tests looking good in this code before moving all of them.

I am not sure I understand this feedback either (maybe my english parser is broken) Maybe a short call would make it easier?

jacques-n · 2024-03-21T16:37:34Z

bootstrap/004_labels.sql

+        -- label name must be unique across all group_names
+        ('(select count(*) = 0 from internal.labels where group_name = f:name and group_name is not null)', 'A label group already exists with this name', true, true),
+        -- Condition must compile
+        ('with result as procedure (input varchar) returns boolean language sql as \$\$ begin let c varchar := (select parse_json(:input):condition);execute immediate \'select case when \' || :c || \' then true else false end from reporting.enriched_query_history limit 1\';return true;end;\$\$ call result(?);', 'Label condition failed to compile', false, false),


Not like the others. Should come up with different pattern.

bootstrap/0036_sql_tools.sql

joshelser · 2024-04-10T20:02:21Z

test/unit/test_label_procs.py

+    # Create a label with this name
+    old_label = generate_unique_name("old_label", timestamp_string)
+    sql = f"call ADMIN.CREATE_LABEL('{old_label}', NULL, NULL, 'rows_produced > 100');"
+    assert run_proc(conn, sql) is None, "ADMIN.CREATE_LABEL did not return NULL value!"
+
+    # Create a dynamic label
+    old_dyn_label = generate_unique_name("old_dyn_label", timestamp_string)
+    sql = f"call ADMIN.CREATE_LABEL(NULL, '{old_dyn_label}', NULL, 'QUERY_TYPE', TRUE);"
+    assert run_proc(conn, sql) is None, "ADMIN.CREATE_LABEL did not return NULL value!"

    label = generate_unique_name("label", timestamp_string)
-    sql = statement.format(label=label)
+    sql = statement.format(
+        label=label, old_label=old_label, old_dyn_label=old_dyn_label
+    )


The python label implementation validated the conditions first and then verified existing labels against the input (e.g. cannot create a label with the same name as one that exists, a label has to exist with the name being updated).

I can separate this change out to its own PR, but hopefully this is clear enough.

joshelser · 2024-04-11T01:50:56Z

Pushed a few commits:

admin.write(..) now incrementally builds up the "pieces" from the incoming object, to then combine into the skeleton of the MERGE statement. This is much more readable to me.
Tried to capture the obvious validation (is not, is not null) as UDFs that generate the condition.
I changed over the validations for ungrouped labels to creating a single object, and updated admin.validate to use those.

I didn't migrate the grouped labels and dynamic labels validations (so those tests will fail). I wanted to see what you two thought.

rymurr

I wonder if we can split this into a few PRs. Just having trouble reviewin geveyrhting,

rymurr · 2024-04-12T10:56:52Z

bootstrap/004_labels.sql

+-- Ungrouped labels
+-- simple=true and create_only=false as the defaults
+CREATE OR REPLACE VIEW INTERNAL.VALIDATE_LABELS AS
+    select $1 as obj from (


this seems a bit weird. Could we use VALUES isntead of a union all?

Frustratingly, not with the UDFs as the code currently has (to my knowledge). You can only use values when each value evaluates to a constant expression at compilation time.

Ok, yes, Snowflake can collapse the UDF to a constant expression so I could switch this back to using VALUES but you can't use object_construct() in a VALUES statement.

rymurr · 2024-04-12T10:59:05Z

bootstrap/004_labels.sql

+CREATE OR REPLACE VIEW INTERNAL.VALIDATE_GROUPED_LABELS AS
+    select $1 as sql, $2 as message, $3 as simple, $4 as create_only from (
+        -- Basic null checks, converting variant null to sql null
+        SELECT VALIDATION.NOT_NULL('group_name'), 'Group name must not be null', true, false


Urgh, its annoying we have these 3 types and so many duplicate checks across them

rymurr · 2024-04-12T10:59:29Z

bootstrap/004_labels.sql

+        SELECT '(select count(*) = 0 from internal.labels where group_name = f:group_name and group_rank = f:group_rank)', 'A label already already exists with this rank', true, true
+        UNION ALL
+        -- Condition must compile
+        SELECT 'with result as procedure (input varchar) returns boolean language sql as \$\$ begin let c varchar := (select parse_json(:input):condition);execute immediate \'select substring(\' || :c || \', 0, 0)  from reporting.enriched_query_history where false\';return true;end;\$\$ call result(?);', 'Invalid label condition', false, false


I think we have to do something about these complex checks. Lets discuss in our sync up

joshelser · 2024-04-13T20:16:23Z

@rymurr feedback from friday. Notably, I dropped grouped and dynamic label stuff from the current PR to reduce the size of it. One addition that we didn't discuss was that I also dropped the overloaded CREATE_LABEL and UPDATE_LABEL procedures (making is_dynamic default false lets us avoid the same blocks of code twice).

The UPDATE_LABEL procedure is still complicated -- I feel like the clean way to address complexity there is to make separate procedures for other labels, rather than overloading into singular procedures.

rymurr

I like this! Looking goood. Can we get some accurate timings for this?

rymurr · 2024-04-15T09:39:03Z

bootstrap/0036_sql_tools.sql

+$$
+begin
+    -- Select the validation rows. In a create, choose all. In an update, omit rows which are for create_only.
+    let validation_query text := (select tools.templatejs(


in the interest of fewer calls to js I am not sure if we need this. I would maybe do smoething like:

let validation_query text := 'select * from identifier('internal.validate_' || ?) where iff(?, true, NOT COALESCE(obj[\'create_only\'], FALSE))' let rs resultset := (execute immediate :validation_query using (:validation_table, :is_create));

bootstrap/0036_sql_tools.sql

rymurr · 2024-04-15T09:41:52Z

bootstrap/004_labels.sql

+AS
+BEGIN
+    let c varchar := (select parse_json(:input):condition);
+    let stmt varchar := (select 'select case when ' || :c || ' then true else false end from reporting.enriched_query_history limit 1');


this coul duse execute immediate stmt using (c) which would simplify the concat above

I'm not sure we can use execute immediate in this case. c is just a string, but the case statement in the query needs to be a condition which evaluates to a boolean.

I can't think of any tricks to your suggestion work (I thought string concat to build the query is the only solution). Have I missed one?

rymurr · 2024-04-15T09:42:02Z

bootstrap/004_labels.sql

+    let stmt varchar := (select 'select ' || :n || ' from reporting.enriched_query_history where false');
+    execute immediate stmt;
+    return false;


rymurr · 2024-04-15T09:42:28Z

bootstrap/004_labels.sql

+    let c varchar := (select parse_json(:input):condition);
+    let stmt varchar := (select 'select case when ' || :c || ' then true else false end from reporting.enriched_query_history limit 1');
+    execute immediate stmt;
+    return true;


should this return the output of execute immediate?

i think we rarely hit the else branch (often raise an exception), but yes that would be correct to do.

oh, no, that's not right. We are just testing the compilation here. The actual row we get from query history may evaluate to true or false..

bootstrap/004_labels.sql

joshelser · 2024-04-16T01:45:06Z

bootstrap/0036_sql_tools.sql

+    let rs resultset := (select false);
+    return table(rs);
+EXCEPTION
+    WHEN STATEMENT_ERROR THEN
+        let rs resultset := (select true);
+        return table(rs);
+    WHEN OTHER then
+        let rs resultset := (select false);
+        return table(rs);


I had to change this proc and the previous one to returning a table so https://github.com/sundeck-io/OpsCenter/pull/591/files#diff-4d8db5ec382ac980083e32c616facb93597373fbc86135753e973a6fb38493a3R30 works w/o special handling.

I could capture the boolean and have a single table(rs) at the very end, but I really wish I didn't have to.

Short of wrapping a anonymous procedure around the real procedure, I couldn't come up with a way to "rename" the output column from the proc (the column name of the result is always the name of the procedure itself).

rymurr

1 minor question and seems tests are still failing. I think it looks great though.

Any comments on time and improvements?

rymurr · 2024-04-16T08:51:24Z

bootstrap/0036_sql_tools.sql

+    let c varchar := (select parse_json(:input):condition);
+    let stmt varchar := (select 'select ' || :c || ' from reporting.enriched_query_history limit 1');
+    execute immediate :stmt;
+    -- if the condition is invalid, we let the exception propagate.


why do we allow the exception to propagate here but below we capture it?

We don't managed the exceptions below so we can distinguish statement_error from other exceptions. In this procedure, any exception indicates a failed validation (and we can let the caller handle it since we have the caller doing this as a fail-safe).

SELECT VALUES must give constant values. Switch the view validation statements to select and union all since the UDF is not constant expression.

* Remove templatejs from validation UDFs as they cannot be used in VALUES statements. * Extract anonymous procedures into standalone procs, use those in label validation * Fiddle with the validation view. Also can't use OBJECT_CONSTRUCT in a VALUES expression.

…ouped and dynamic label stuff now to trim the PR size

* can't concat inside `identifier` * boolean somehow became a text through `execute immediate .. using`

joshelser · 2024-04-16T18:25:24Z

Any comments on time and improvements?

Yeesh, we took a step backwards for ungrouped label creation

ADMIN.CREATE_LABEL() is taking ~6s. Of that, the current breakdown is:

admin.validate takes ~3.2s
admin.write takes ~1.2s
internal.update_label_view takes ~1.8s

The sum of queries executed by admin.write are around 700ms, so 1.2s seems long. The two "complex" validations represent ~50% of the total 3.2s of validation. I'll see if there's something obvious we can do differently here.

I'll re-evaluate with a hybrid table in a momen.t

joshelser · 2024-04-16T18:31:32Z

I'll re-evaluate with a hybrid table in a moment

Making internal.labels a hybrid table with label_id primary key drops the execution time of the MERGE statement inside admin.write from ~400ms to ~250ms.

joshelser · 2024-04-16T21:10:36Z

bootstrap/004_labels.sql

+        ('CALL VALIDATION.VALID_LABEL_CONDITION(?)', '{"message": "Invalid label condition", "complex": true}'),
+        -- make sure label name doesn't exist as query history column
+        ('CALL VALIDATION.IS_VALID_LABEL_NAME(?)', '{"message": "Label name cannot be the same as a column in REPORTING.ENRICHED_QUERY_HISTORY", "complex": true}')


These two checks as standalone procedures (versus inline blocks) cost ~1s.

jacques-n · 2024-04-16T21:12:15Z

I think we should be able to make all validation a single SQL select query. Prove me wrong :D

joshelser · 2024-04-16T21:20:45Z

I think we should be able to make all validation a single SQL select query. Prove me wrong :D

I just pushed the latest (which has grouped and ungrouped labels passing their tests, but not dynamic labels). I did a quick prototype with just the simple validations in one select but ignored the "complex" validations (which we know for sure are the time-suck).

I'll see if I can get all of the checks into a single query. The one problem I can immediately think of is that we rely on an exception to be raised to verify that a label name doesn't duplicate a name in enriched_query_history. Will have to take a different approach on that check.

rymurr · 2024-04-17T09:38:17Z

Maybe an unpopular opinion: should we give this up and just go back to V1 of these? V1 beign specialized procedures for each crud type? I seem to recall those being faster than this. My memory:

v1: First we had standalone hand coded procs for each entity type. Reasonably fast but hard to extend and had to be called as stored procs from streamlit
v2: Move into a generic python structure. Easy to extend and very fast from streamlit but slow from stored procs
v3: since we are getting rid of streamlit we don't care if its fast from streamlit. We need fast from stored procs. Do we need it to be easy to extend?

My suggestion, in the interest of expediency is to either defer this whole problem or to stop trying to make generic validation. Thoughts?

joshelser · 2024-04-17T13:03:13Z

Maybe an unpopular opinion: should we give this up and just go back to V1 of these? V1 beign specialized procedures for each crud type?

I had the same thought yesterday. I almost installed an old version just to confirm that they were actually faster before.

joshelser requested review from rymurr and vqmarkman March 20, 2024 01:39

joshelser force-pushed the sql-label-procs branch from 85733dc to bbf646c Compare March 20, 2024 15:45

joshelser requested a review from jacques-n March 21, 2024 14:52

jacques-n reviewed Mar 21, 2024

View reviewed changes

joshelser force-pushed the sql-label-procs branch from bbf646c to ebe3a1b Compare April 1, 2024 01:16

joshelser commented Apr 10, 2024

View reviewed changes

joshelser force-pushed the sql-label-procs branch from ebe3a1b to b28e4c9 Compare April 11, 2024 01:45

rymurr reviewed Apr 12, 2024

View reviewed changes

rymurr reviewed Apr 15, 2024

View reviewed changes

joshelser commented Apr 16, 2024

View reviewed changes

rymurr previously approved these changes Apr 16, 2024

View reviewed changes

joshelser added 12 commits April 16, 2024 13:55

Only product changes, no test changes

7a2e9db

Avoid creating a table

2c8b52c

Get tests passing again

e29ee1a

Create validation UDFs which use templatejs

3ab0b95

SELECT VALUES must give constant values. Switch the view validation statements to select and union all since the UDF is not constant expression.

Refactor admin.write to build up the merge statement.

8e439a3

Change ungrouped labels valiations to an object

d44b978

Code review from Ryan.

f9ba8ba

* Remove templatejs from validation UDFs as they cannot be used in VALUES statements. * Extract anonymous procedures into standalone procs, use those in label validation * Fiddle with the validation view. Also can't use OBJECT_CONSTRUCT in a VALUES expression.

Drop the overloaded create and update procedures. Drop some of the gr…

0a4aecd

…ouped and dynamic label stuff now to trim the PR size

code review

bfb57a5

fixup

e1b646b

* can't concat inside `identifier` * boolean somehow became a text through `execute immediate .. using`

move new funcs into 0036

f0f6b99

Re-add grouped labels

8fefa04

joshelser added 2 commits April 16, 2024 13:55

Reintroduce grouped label validation

75b513f

All validators need to return a result boolean column

232b96b

joshelser commented Apr 16, 2024

View reviewed changes

joshelser added 2 commits April 16, 2024 17:14

Remove some duplicated code

2fff52a

Create labels table as hybrid table

c69445f

joshelser dismissed rymurr’s stale review via c69445f April 16, 2024 21:16

joshelser force-pushed the sql-label-procs branch from 704d76e to c69445f Compare April 16, 2024 21:16

SQL stored procs for labels to replace python procs #591

Are you sure you want to change the base?

SQL stored procs for labels to replace python procs #591

Conversation

joshelser commented Mar 20, 2024

joshelser commented Mar 20, 2024

joshelser commented Mar 21, 2024

joshelser commented Mar 21, 2024

jacques-n commented Mar 21, 2024

jacques-n Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshelser commented Apr 11, 2024

rymurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshelser commented Apr 13, 2024

rymurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rymurr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshelser commented Apr 16, 2024

joshelser commented Apr 16, 2024

Choose a reason for hiding this comment

jacques-n commented Apr 16, 2024

joshelser commented Apr 16, 2024 • edited Loading

rymurr commented Apr 17, 2024

joshelser commented Apr 17, 2024

jacques-n Mar 21, 2024 •

edited

Loading

joshelser commented Apr 16, 2024 •

edited

Loading