CICD bot diff preview not showing change to macro variable

[plaflamme]

I have a model with a local variable defined:

MODEL(...)
@DEF(
    cols, 
    [a,b,c]
);
...

When adding an item to the list, e.g.:

MODEL(...)
@DEF(
  cols,
  [a,b,c,d]
)
...

The rendered diff preview looks like this in the Github comment:

--- 

+++ 

@@ -1 +1 @@

-@DEF(cols, )
+@DEF(cols, )

[eakmanrq]

@georgesittas There shouldn't be anything specific to CI/CD bot that would create this behavior. So if your PR fixed the diff in normal plan/apply output then it should also be fixed in the bot. Is that the case?

[georgesittas]

I couldn't repro this in the CLI after checking out to the parent commit of https://github.com/TobikoData/sqlmesh/commit/8f25d5be55e026ee94b710edc9381fdb1ca5306f:

➜  test sqlmesh init duckdb
➜  test sqlmesh render sqlmesh_example.full_model  # Added the DEF to the full model first
SELECT
  "incremental_model"."item_id" AS "item_id",
  COUNT(DISTINCT "incremental_model"."id") AS "num_orders",
  1 IN (1, 2, 3, 4) AS "_col_2"
FROM "sqlmesh_example"."incremental_model" AS "incremental_model"
GROUP BY
  "incremental_model"."item_id"
ORDER BY
  "item_id"
➜  test sqlmesh plan
...
➜  test sqlmesh render sqlmesh_example.full_model
SELECT
  "incremental_model"."item_id" AS "item_id",
  COUNT(DISTINCT "incremental_model"."id") AS "num_orders",
  1 IN (1, 2, 3, 4, 5) AS "_col_2"
FROM "sqlmesh__sqlmesh_example"."sqlmesh_example__incremental_model__3958293195" AS "incremental_model"
GROUP BY
  "incremental_model"."item_id"
ORDER BY
  "item_id"
➜  test sqlmesh plan
======================================================================
Successfully Ran 0 tests against duckdb
----------------------------------------------------------------------
Summary of differences against `prod`:
Models:
└── Directly Modified:
    └── sqlmesh_example.full_model
---

+++

@@ -5,7 +5,7 @@

   audits (ASSERT_POSITIVE_ORDER_IDS()),
   grains (item_id)
 )
-@DEF(vals, [1, 2, 3, 4])
+@DEF(vals, [1, 2, 3, 4, 5])
 SELECT
   item_id,
   COUNT(DISTINCT id) AS num_orders,

[plaflamme]

Indeed, the cli output was showing the right thing. It's only the diff in the GH comment that was off. I can try to reproduce this if necessary

[georgesittas]

@plaflamme a reproducible example / sequence would be helpful I think. If what I did above also works for reproducing the issue in the PR bot, then we can work with that.

[plaflamme]

My model looks like this:

-- SCD Type 2
MODEL(
    name ****,
    kind FULL,
    cron "@daily",
    start "2020-01-01",
);
@DEF(
    unique_key, 
    [a,b,c]
);
@DEF(
    cols, 
    [d,e,f,g,h]
);
WITH prev AS(
...
)
SELECT ...

(the actual column names are longer).

If I remove the h column from the second @DEF, I see the following in the PR comment

The problem is the same when I add the column back...

I have SQLMESH_DEBUG=1 in the cicd bot and I can see this in the logs:

2023-12-04 20:33:08,374 - MainThread - sqlmesh.integrations.github.cicd.controller - DEBUG - Updating check with kwargs: {'name': 'SQLMesh - Prod Environment Synced', 'head_sha': '32743c33c320efdb32de1f759c2dcacb240c9529', 'status': 'completed', 'completed_at': datetime.datetime(2023, 12, 4, 20, 33, tzinfo=datetime.timezone.utc), 'conclusion': 'success', 'output': {'title': 'Deployed to Prod', 'summary': '**Generated Prod Plan**\n```diff\n--- \n\n+++ \n\n@@ -1 +1 @@\n\n-@DEF(cols, )\n+@DEF(cols, )\n```\n\n```\n\nDirectly Modified: **** }} (controller.py:604)

So something is stripping away that part of the diff before it is submitted to GH.

The output of sqlmesh plan is fine. I'm on 0.57.0

[georgesittas]

@plaflamme thanks for the detailed report, it's interesting that the diff is being truncated and probably a good hint. I tried to repro this again locally in an integration test using a sushi model but the diff was correct for me. Granted, my example was slightly different compared to yours:

diff --git a/examples/sushi/models/waiter_revenue_by_day.sql b/examples/sushi/models/waiter_revenue_by_day.sql
index ac8ac50c..052a0942 100644
--- a/examples/sushi/models/waiter_revenue_by_day.sql
+++ b/examples/sushi/models/waiter_revenue_by_day.sql
@@ -13,10 +13,16 @@ MODEL (
   grain (waiter_id, event_date)
 );

+@DEF(
+    cols,
+    ['d','e','f','g','h']
+);
+
 SELECT
   o.waiter_id::INT AS waiter_id, /* Waiter id */
   SUM(oi.quantity * i.price)::DOUBLE AS revenue, /* Revenue from orders taken by this waiter */
-  o.event_date::DATE AS event_date /* Date */
+  o.event_date::DATE AS event_date, /* Date */
+  @cols::ARRAY<VARCHAR> AS cols,
 FROM sushi.orders AS o
 LEFT JOIN sushi.order_items AS oi
   ON o.id = oi.order_id AND o.event_date = oi.event_date

But I wouldn't expect this to affect the output significantly. I basically upserted the same same model in the test's context with the @DEF having changed so as to not contain 'h'.

Any chance you can try this out once again with a clean state using the current main branch to ensure it's still happening? If it does, I'll try to pair with Ryan sometime soon and see if we can figure it out.

[tobymao]

@plaflamme are you still able to reproduce this?

[plaflamme]

@tobymao unfortunately, yes. Here's a screenshot of the "Prod Plan Preview" in GitHub for a PR that removes one item from the list.

FWIW: this isn't a big concern

[georgesittas]

@plaflamme any chance you can check if this is still reproducible?

[plaflamme]

Yes, still seeing this on occasion in 0.136.1

still not a big deal...

[georgesittas]

Interesting, thanks for checking this (again). Maybe we'll take another look at this. Been addressing tech debt lately :)