Repository: ibis-project/ibis

Issue:
feat: warn when limit reached in expr.execute()
When executing an expression a limit is silently applied to what is returned (unless you pass `limit=None`). A user could accidentally continue with local analysis on an arbitrary subset without realizing it's a subset. I'd like an optional parameter/setting that causes it to write a warning to `stderr` or raise an exception if the limit was exceeded. 

Since the limit is currently implemented by adding a limit expression to the original query, it may be difficult to know whether the limit would have been exceeded. In that case, maybe it could just check to see if the number of returned records equals the limit and issue the warning then. It might not definitively mean that the limit would've been exceeded, but would still be useful to get a warning that the limit was potentially exceeded. 

Discussion:
github-actions[bot]: ## Test Results
       6 files         6 suites   4m 53s [:stopwatch:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "duration of all tests")
3 248 tests 3 231 [:heavy_check_mark:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "passed tests") 17 [:zzz:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "skipped / disabled tests") 0 [:x:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "failed tests")
9 717 runs  9 666 [:heavy_check_mark:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "passed tests") 51 [:zzz:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "skipped / disabled tests") 0 [:x:](https://github.com/EnricoMi/publish-unit-test-result-action/blob/v1.20/README.md#the-symbols "failed tests")

Results for commit b3cf508f.

[test-results]:data:application/gzip;base64,H4sIAJmoSWMC/03MTQ7CIBCG4as0rF0wxQLjZQxFJiH2x1BYNd5dQItdvs9kvp2Rn9zGbp28dGxLPrZ4pGCiX5ecPYoM+RTLUfRXfeR9S9ZWE/C3p39lA9WEjJ+y8AYuhDX8JKSlzKKqD6WOVZRSNvqODnDAabP2edKu8+xjDjYKSwPXRI5ziaTQajCjUA6d0iBHThytU8DeH4+I4doLAQAA

codecov[bot]: # [Codecov](https://codecov.io/gh/ibis-project/ibis/pull/4650?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project) Report
> Merging [#4650](https://codecov.io/gh/ibis-project/ibis/pull/4650?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project) (b3cf508) into [master](https://codecov.io/gh/ibis-project/ibis/commit/498109d0dfe891d048c0c39881c0271fc8cce24a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project) (498109d) will **decrease** coverage by `2.03%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/ibis-project/ibis/pull/4650/graphs/tree.svg?width=650&height=150&src=pr&token=sE9aT1Le0L&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project)](https://codecov.io/gh/ibis-project/ibis/pull/4650?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project)

```diff
@@            Coverage Diff             @@
##           master    #4650      +/-   ##
==========================================
- Coverage   92.44%   90.40%   -2.04%     
==========================================
  Files         192      192              
  Lines       21373    21373              
  Branches     2950     2950              
==========================================
- Hits        19758    19322     -436     
- Misses       1203     1639     +436     
  Partials      412      412              
```

| [Impacted Files](https://codecov.io/gh/ibis-project/ibis/pull/4650?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project) | Coverage Δ | |
|---|---|---|
| [ibis/config.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9jb25maWcucHk=) | `93.97% <100.00%> (-6.03%)` | :arrow_down: |
| [ibis/backends/duckdb/registry.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9kdWNrZGIvcmVnaXN0cnkucHk=) | `32.29% <0.00%> (-60.42%)` | :arrow_down: |
| [ibis/backends/sqlite/\_\_init\_\_.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9zcWxpdGUvX19pbml0X18ucHk=) | `39.72% <0.00%> (-56.17%)` | :arrow_down: |
| [ibis/backends/snowflake/datatypes.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9zbm93Zmxha2UvZGF0YXR5cGVzLnB5) | `36.36% <0.00%> (-53.04%)` | :arrow_down: |
| [ibis/backends/snowflake/\_\_init\_\_.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9zbm93Zmxha2UvX19pbml0X18ucHk=) | `44.26% <0.00%> (-44.27%)` | :arrow_down: |
| [ibis/backends/snowflake/registry.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9zbm93Zmxha2UvcmVnaXN0cnkucHk=) | `50.00% <0.00%> (-43.34%)` | :arrow_down: |
| [ibis/backends/sqlite/registry.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9zcWxpdGUvcmVnaXN0cnkucHk=) | `53.44% <0.00%> (-42.25%)` | :arrow_down: |
| [ibis/backends/duckdb/\_\_init\_\_.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9kdWNrZGIvX19pbml0X18ucHk=) | `44.93% <0.00%> (-41.14%)` | :arrow_down: |
| [ibis/backends/sqlite/udf.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9zcWxpdGUvdWRmLnB5) | `54.08% <0.00%> (-38.78%)` | :arrow_down: |
| [ibis/backends/duckdb/datatypes.py](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project#diff-aWJpcy9iYWNrZW5kcy9kdWNrZGIvZGF0YXR5cGVzLnB5) | `64.28% <0.00%> (-35.72%)` | :arrow_down: |
| ... and [14 more](https://codecov.io/gh/ibis-project/ibis/pull/4650/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=ibis-project) | |

cpcloud: I'll merge this after #4726, since that should address the `simulate_release` failure with no changes to this PR.

Patch:
diff --git a/ibis/backends/dask/execution/strings.py b/ibis/backends/dask/execution/strings.py
index 0b471eac0..dbd187391 100644
--- a/ibis/backends/dask/execution/strings.py
+++ b/ibis/backends/dask/execution/strings.py
@@ -54,7 +54,7 @@ DASK_DISPATCH_TYPES: TypeRegistrationDict = {
             (
                 dd.Series,
                 integer_types,
-                integer_types,
+                (type(None), *integer_types),
             ),
             execute_substring_int_int,
         ),
diff --git a/ibis/backends/duckdb/registry.py b/ibis/backends/duckdb/registry.py
index 6624cb9fa..747477d88 100644
--- a/ibis/backends/duckdb/registry.py
+++ b/ibis/backends/duckdb/registry.py
@@ -144,6 +144,15 @@ def _regex_extract(t, op):
     return result
 
 
+def _json_get_item(t, op):
+    left, path = map(t.translate, op.args)
+    # Workaround for https://github.com/duckdb/duckdb/issues/5063
+    # In some situations duckdb silently does the wrong thing if
+    # the path is parametrized.
+    sa_path = sa.text(str(path.compile(compile_kwargs=dict(literal_binds=True))))
+    return left.op("->")(sa_path)
+
+
 def _strftime(t, op):
     format_str = op.format_str
     if not isinstance(format_str_op := format_str, ops.Literal):
@@ -220,6 +229,7 @@ operation_registry.update(
         ops.ArgMin: reduction(sa.func.min_by),
         ops.ArgMax: reduction(sa.func.max_by),
         ops.BitwiseXor: fixed_arity(sa.func.xor, 2),
+        ops.JSONGetItem: _json_get_item,
     }
 )
 
diff --git a/ibis/backends/pandas/execution/strings.py b/ibis/backends/pandas/execution/strings.py
index f507b001e..dd8b37daa 100644
--- a/ibis/backends/pandas/execution/strings.py
+++ b/ibis/backends/pandas/execution/strings.py
@@ -21,9 +21,14 @@ def execute_string_length_series(op, data, **kwargs):
     return data.str.len().astype('int32')
 
 
-@execute_node.register(ops.Substring, pd.Series, integer_types, integer_types)
+@execute_node.register(
+    ops.Substring, pd.Series, integer_types, (type(None), *integer_types)
+)
 def execute_substring_int_int(op, data, start, length, **kwargs):
-    return data.str[start : start + length]
+    if length is None:
+        return data.str[start:]
+    else:
+        return data.str[start : start + length]
 
 
 @execute_node.register(ops.Substring, pd.Series, pd.Series, integer_types)
diff --git a/ibis/config.py b/ibis/config.py
index 6514c3078..1cf774855 100644
--- a/ibis/config.py
+++ b/ibis/config.py
@@ -67,7 +67,7 @@ class SQL(Config):
         Dialect to use for printing SQL when the backend cannot be determined.
     """
 
-    default_limit: Optional[PosInt] = 10_000
+    default_limit: Optional[PosInt] = None
     default_dialect: str = "duckdb"
 
 
diff --git a/ibis/expr/builders.py b/ibis/expr/builders.py
index 91fe8f996..c42eb95bc 100644
--- a/ibis/expr/builders.py
+++ b/ibis/expr/builders.py
@@ -66,7 +66,10 @@ class SimpleCaseBuilder(TypedCaseBuilder):
         result_expr = rlz.any(result_expr)
 
         if not rlz.comparable(self.base, case_expr):
-            raise TypeError('Base expression and passed case are not ' 'comparable')
+            raise TypeError(
+                f'Base expression {rlz._arg_type_error_format(self.base)} and '
+                f'case {rlz._arg_type_error_format(case_expr)} are not comparable'
+            )
 
         cases = list(self.cases)
         cases.append(case_expr)
diff --git a/ibis/expr/operations/logical.py b/ibis/expr/operations/logical.py
index e5c56d615..265577ee0 100644
--- a/ibis/expr/operations/logical.py
+++ b/ibis/expr/operations/logical.py
@@ -63,8 +63,8 @@ class Comparison(Binary):
         """
         if not rlz.comparable(left, right):
             raise TypeError(
-                f'Arguments with datatype {left.output_dtype} and '
-                f'{right.output_dtype} are not comparable'
+                f'Arguments {rlz._arg_type_error_format(left)} and '
+                f'{rlz._arg_type_error_format(right)} are not comparable'
             )
         super().__init__(left=left, right=right)
 
@@ -116,13 +116,13 @@ class Between(Value):
     def __init__(self, arg, lower_bound, upper_bound):
         if not rlz.comparable(arg, lower_bound):
             raise TypeError(
-                f'Argument with datatype {arg.output_dtype} and lower bound '
-                f'with datatype {lower_bound.output_dtype} are not comparable'
+                f'Arguments {rlz._arg_type_error_format(arg)} and '
+                f'{rlz._arg_type_error_format(lower_bound)} are not comparable'
             )
         if not rlz.comparable(arg, upper_bound):
             raise TypeError(
-                f'Argument with datatype {arg.output_dtype} and upper bound '
-                f'with datatype {upper_bound.output_dtype} are not comparable'
+                f'Arguments {rlz._arg_type_error_format(arg)} and '
+                f'{rlz._arg_type_error_format(upper_bound)} are not comparable'
             )
         super().__init__(arg=arg, lower_bound=lower_bound, upper_bound=upper_bound)
 
diff --git a/ibis/expr/rules.py b/ibis/expr/rules.py
index d234e4da0..c2dd83741 100644
--- a/ibis/expr/rules.py
+++ b/ibis/expr/rules.py
@@ -585,3 +585,12 @@ def window_from(table_ref, win, **kwargs):
         if not isinstance(order_var.output_dtype, dt.Timestamp):
             raise com.IbisInputError(error_msg)
     return win
+
+
+def _arg_type_error_format(op):
+    from ibis.expr.operations.generic import Literal
+
+    if isinstance(op, Literal):
+        return f"Literal({op.value}):{op.output_dtype}"
+    else:
+        return f"{op.name}:{op.output_dtype}"


Test patch:
diff --git a/ibis/backends/sqlite/tests/test_client.py b/ibis/backends/sqlite/tests/test_client.py
index ece045e0f..d28315618 100644
--- a/ibis/backends/sqlite/tests/test_client.py
+++ b/ibis/backends/sqlite/tests/test_client.py
@@ -83,10 +83,7 @@ def test_verbose_log_queries(con):
 
     assert len(queries) == 1
     (query,) = queries
-    expected = 'SELECT t0.year \n'
-    expected += 'FROM main.functional_alltypes AS t0\n'
-    expected += ' LIMIT ? OFFSET ?'
-    assert query == expected
+    assert "SELECT t0.year" in query
 
 
 def test_table_equality(dbpath):
diff --git a/ibis/backends/tests/test_string.py b/ibis/backends/tests/test_string.py
index 668d08cee..948c39ff6 100644
--- a/ibis/backends/tests/test_string.py
+++ b/ibis/backends/tests/test_string.py
@@ -204,6 +204,12 @@ def test_string_col_is_unicode(alltypes, df):
             lambda t: t.date_string_col.str[2:5],
             id='substr',
         ),
+        param(
+            lambda t: t.date_string_col.substr(2),
+            lambda t: t.date_string_col.str[2:],
+            id='substr-start-only',
+            marks=pytest.mark.notimpl(["datafusion", "polars"]),
+        ),
         param(
             lambda t: t.date_string_col.left(2),
             lambda t: t.date_string_col.str[:2],
diff --git a/ibis/tests/expr/test_value_exprs.py b/ibis/tests/expr/test_value_exprs.py
index 6cd36fb1a..d798a62e7 100644
--- a/ibis/tests/expr/test_value_exprs.py
+++ b/ibis/tests/expr/test_value_exprs.py
@@ -1623,3 +1623,29 @@ def test_numpy_ufuncs_dont_cast_columns():
     x = np.int64(1)
     for expr in [t.a + x, x + t.a]:
         assert expr.equals(t.a + ibis.literal(x))
+
+
+@pytest.mark.parametrize(
+    'operation',
+    [
+        operator.lt,
+        operator.gt,
+        operator.ge,
+        operator.le,
+        operator.eq,
+        operator.ne,
+    ],
+)
+def test_logical_comparison_rlz_incompatible_error(table, operation):
+    with pytest.raises(TypeError) as exc_info:
+        operation(table.b, "foo")
+
+    assert "b:int16 and Literal(foo):string" in str(exc_info.value)
+
+
+def test_case_rlz_incompatible_error(table):
+    case, result = "foo", table.a
+    with pytest.raises(TypeError) as exc_info:
+        table.g.case().when(case == result, result).end()
+
+    assert "a:int8 and Literal(foo):string" in str(exc_info.value)
diff --git a/ibis/tests/test_config.py b/ibis/tests/test_config.py
index 75f7be40d..ad3f45e1b 100644
--- a/ibis/tests/test_config.py
+++ b/ibis/tests/test_config.py
@@ -4,11 +4,13 @@ from ibis.config import options
 
 
 def test_sql_config():
-    assert options.sql.default_limit == 10_000
+    try:
+        assert options.sql.default_limit is None
 
-    with pytest.raises(TypeError):
-        options.sql.default_limit = -1
+        with pytest.raises(TypeError):
+            options.sql.default_limit = -1
 
-    options.sql.default_limit = 100
-    assert options.sql.default_limit == 100
-    options.sql.default_limit = 10_000
+        options.sql.default_limit = 100
+        assert options.sql.default_limit == 100
+    finally:
+        options.sql.default_limit = None