Repository: ploomber/jupysql
Pull request #496 (2023-05-18 16:55:45)

variable expansion fix
## Describe your changes
Fixed user guide on variable expansion

## Issue number

Closes 

## Checklist before requesting a review

- [x] Performed a self-review of my code
- [x] Formatted my code with [`pkgmt format`](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#linting-formatting)
- [x] Added [tests](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#testing) (when necessary).
- [x] Added [docstring](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#documenting-changes-and-new-features) documentation and update the [changelog](https://ploomber-contributing.readthedocs.io/en/latest/contributing/pr.html#changelog) (when needed)



<!-- readthedocs-preview jupysql start -->
----
:books: Documentation preview :books:: https://jupysql--496.org.readthedocs.build/en/496/

<!-- readthedocs-preview jupysql end -->

neelasha23: With DuckDB version 0.7.1 the below query returns [(None, 1), (18.76, 1), (32.760000000000005, 1)] when data contains null values:
```
select
floor("age"/0.28)*0.28 as bin,
count(*) as count
from "data.csv"
group by bin
order by bin;
```

But with the latest DuckDB release, the order gets changed to [(18.76, 1), (32.760000000000005, 1), (None, 1)] for the same set of query and data. So modified the condition accordingly.

@edublancas @idomic 
neelasha23: Edit on the `image_comparison` failure: It looks like the  data returned from `_histogram_stacked` function is behaving differently with the latest release of DuckDB on Mac. For one of the test cases previously it was returning 

```
[('Ideal', 11020, 4002, 2162, 1349, 939, 696, 499, 335, 267, 249, 33), ('Premium', 5179, 2173, 2337, 1305, 760, 548, 462, 405, 305, 284, 33), ('Good', 1758, 1058, 1025, 420, 214, 118, 94, 81, 76, 53, 9), ('Very Good', 4775, 2419, 1998, 1012, 570, 407, 307, 210, 196, 164, 24), ('Fair', 346, 547, 348, 142, 66, 44, 40, 32, 27, 14, 4)]
```

and with DuckDB 0.8.0 the same function and query is returning 
```
[('Very Good', 4775, 2419, 1998, 1012, 570, 407, 307, 210, 196, 164, 24), ('Good', 1758, 1058, 1025, 420, 214, 118, 94, 81, 76, 53, 9), ('Premium', 5179, 2173, 2337, 1305, 760, 548, 462, 405, 305, 284, 33), ('Fair', 346, 547, 348, 142, 66, 44, 40, 32, 27, 14, 4), ('Ideal', 11020, 4002, 2162, 1349, 939, 696, 499, 335, 267, 249, 33)]
```

For now should I add the `tol` parameter in `image_comparison` for Mac ?
edublancas: can you show both versions of the plots? I wouldn't expect the order of the data to have any effect on the plots, so the next step to take depends on what are the differences
neelasha23: Image generated now:
<img width="459" alt="Screenshot 2023-05-18 at 11 48 32 PM" src="https://github.com/ploomber/jupysql/assets/29096135/cbe73fde-8891-42de-b831-6f04bb1c4fd9">

Baseline image:
<img width="450" alt="Screenshot 2023-05-18 at 11 48 26 PM" src="https://github.com/ploomber/jupysql/assets/29096135/b0b488a5-c0eb-46f2-96a1-572551089fc9">

edublancas: ok I understand now. the plots are different because since the order is different, they're stacked in a different order.

we need to update the baseline images then.
neelasha23: But the test cases are passing in Windows and Linux
tonykploomber: Do you think we still support this case?
I am getting some issues on this example.

https://jupysql.ploomber.io/en/latest/intro.html

If it's not the simple fix we might need to open another issue to tackle


<img width="823" alt="Screenshot 2023-05-19 at 7 42 12 PM" src="https://github.com/ploomber/jupysql/assets/123580782/3f973da4-7d70-4038-a14c-8a60414e974c">

neelasha23: > Do you think we still support this case? I am getting some issues on this example.
> 
> https://jupysql.ploomber.io/en/latest/intro.html
> 

I guess this wont work now. Do we want use `{{...}}` instead? What do you think @edublancas 



edublancas: @tonykploomber: good catch. we don't support the `$variable` anymore

@neelasha23: this is the action list

- let's pin duckdb 0.7, and report this issue to the duckdb team. maybe this is a and they will fix it
- open another issue so we don't forget that we pinned duckdb
- let's check the docs and see if we have any other places where we have `$variable`
- it seems like even though the notebooks with `$variable` failed, readthedocs passed. If this assumption is correct, then we need to investigate our CI settings, I remember seeing this problem in the past and we fixed it with this [config](https://ploomber-contributing.readthedocs.io/en/latest/maintainer/readthedocs.html), maybe something changed in jupyter-book? we need to ensure that readthedocs fail if any of our notebooks fail

neelasha23: Pinned DuckDB version. Will open an issue in their repo shortly.

> it seems like even though the notebooks with $variable failed, readthedocs passed

`readthedocs` is failing, so replaced `$variable` with `{{variable}}`. Although I'm not sure why it came up in this PR and not the previous ones.

@edublancas 
yafimvo: > Edit on the `image_comparison` failure: It looks like the data returned from `_histogram_stacked` function is behaving differently with the latest release of DuckDB on Mac. For one of the test cases previously it was returning
> 
> ```
> [('Ideal', 11020, 4002, 2162, 1349, 939, 696, 499, 335, 267, 249, 33), ('Premium', 5179, 2173, 2337, 1305, 760, 548, 462, 405, 305, 284, 33), ('Good', 1758, 1058, 1025, 420, 214, 118, 94, 81, 76, 53, 9), ('Very Good', 4775, 2419, 1998, 1012, 570, 407, 307, 210, 196, 164, 24), ('Fair', 346, 547, 348, 142, 66, 44, 40, 32, 27, 14, 4)]
> ```
> 
> and with DuckDB 0.8.0 the same function and query is returning
> 
> ```
> [('Very Good', 4775, 2419, 1998, 1012, 570, 407, 307, 210, 196, 164, 24), ('Good', 1758, 1058, 1025, 420, 214, 118, 94, 81, 76, 53, 9), ('Premium', 5179, 2173, 2337, 1305, 760, 548, 462, 405, 305, 284, 33), ('Fair', 346, 547, 348, 142, 66, 44, 40, 32, 27, 14, 4), ('Ideal', 11020, 4002, 2162, 1349, 939, 696, 499, 335, 267, 249, 33)]
> ```
> 
> For now should I add the `tol` parameter in `image_comparison` for Mac ?

We can order the results in `_histogram_stacked` and make sure we get the same chart in every system (Mac, Win, Linux).
something like this:

```
data = sorted(data, key=lambda x: x[0])
```

We'll still have to change the baseline images
neelasha23: > We can order the results in `_histogram_stacked` and make sure we get the same chart in every system (Mac, Win, Linux). something like this:
> 

This looks like a bug from their end since it behaves differently for different platforms, and this is affecting another flow as well. If we pin duckdb for now as per this [discussion](https://github.com/ploomber/jupysql/pull/496#issuecomment-1554810784) we wont need to change the baseline images.

@yafimvo 



Patch:
diff --git a/doc/api/magic-sql.md b/doc/api/magic-sql.md
index 073bb12..3228e27 100644
--- a/doc/api/magic-sql.md
+++ b/doc/api/magic-sql.md
@@ -205,7 +205,7 @@ FROM my_data
 LIMIT 3
 """
 
-%sql $QUERY
+%sql {{QUERY}}
 ```
 
 ## Templated SQL queries
@@ -228,11 +228,11 @@ limit_two = template.substitute(limit=2)
 **Important:** Ensure you sanitize the input parameters; as malicious parameters will be able to run arbitrary SQL queries.
 
 ```{code-cell} ipython3
-%sql $limit_one
+%sql {{limit_one}}
 ```
 
 ```{code-cell} ipython3
-%sql $limit_two
+%sql {{limit_two}}
 ```
 
 ## Compose large queries
diff --git a/src/sql/run.py b/src/sql/run.py
index 88911d6..db79bef 100644
--- a/src/sql/run.py
+++ b/src/sql/run.py
@@ -179,7 +179,8 @@ class ResultSet(ColumnGuesserMixin):
             yield result
 
     def __str__(self, *arg, **kwarg):
-        self.pretty.add_rows(self)
+        if self.pretty:
+            self.pretty.add_rows(self)
         return str(self.pretty or "")
 
     def __repr__(self) -> str:


Test patch:
diff --git a/src/tests/test_resultset.py b/src/tests/test_resultset.py
index 4fb4dcb..7c82fb1 100644
--- a/src/tests/test_resultset.py
+++ b/src/tests/test_resultset.py
@@ -89,3 +89,12 @@ def test_resultset_with_non_sqlalchemy_results(config):
     conn = duckdb.connect()
     result = conn.execute("SELECT * FROM df")
     assert ResultSet(result, config) == [(0,), (1,), (2,)]
+
+
+def test_none_pretty(config):
+    engine = sqlalchemy.create_engine("sqlite://")
+    conn = engine.connect()
+    result = conn.execute(sqlalchemy.text("create table some_table (name, age)"))
+    result_set = ResultSet(result, config)
+    assert result_set.pretty is None
+    assert "" == str(result_set)