Improved Slicing to support negative indices #277

vineetg3 · 2025-02-21T20:54:03Z

Resolves #260.
Please read the issue for more details.

The SUBSTR conversion logic in convert_slice is being expanded to handle negative indices.
Here is the outline of the logic to support Python based slicing semantics:

Case 1: (None, None)
- Returns the string as is.
Case 2: (start, None)
- Positive start: Convert to 1-based indexing and slice from start.
- Negative start: Compute LENGTH(string) + start + 1; clamp to 1 if less than 1.
Case 3: (None, stop)
- Positive stop: Slice from position 1 to stop.
- Negative stop: Compute LENGTH(string) + stop; clamp to 0 if less than 0 (empty slice).
Case 4: (start, stop)
- 1. Both start & stop >= 0:
  - Convert start to 1-based.
  - Set length = stop - start.
- 1. start < 0, stop >= 0:
  - Convert start to positive. If < 1, set to 1.
  - Compute length = stop - start (clamp to 0 if negative).
- 1. start >= 0, stop < 0:
  - Convert stop & start to positive.
  - If stop < 1, slice is empty (length = 0).
  - Else, length = stop - start.
- 1. start < 0, stop < 0:
  - Convert start & stop to positive. If start < 1, set to 1.
  - If stop < 1, slice is empty (length = 0).
  - Else, length = stop - start.

hadia206

Looks good to me.
I have minor comments.

hadia206 · 2025-02-24T18:12:55Z

pydough/sqlglot/transform_bindings.py

+            - Convert `start` to positive. If < 1, set to 1.
+            - Compute `length = stop - start` (clamp to 0 if negative).
+        - 3. `start >= 0`, `stop < 0`:
+            - Convert `stop` & `start` to positive.


Based on 3. case start is already positive.

Suggested change

- Convert `stop` & `start` to positive.

- Convert `stop` to positive.

hadia206 · 2025-02-24T18:15:25Z

pydough/pydough_operators/expression_operators/README.md

@@ -59,7 +59,7 @@ These are created with a prefix operator syntax instead of called as a function.

 These are other PyDough operators that are not necessarily used as functions:

- `SLICE`: operator used for string slicing, with the same semantics as Python string slicing. If `s[a:b:c]` is done, that is translated to `SLICE(s,a,b,c)` in PyDough, and any of `a`/`b`/`c` could be absent. Currently, PyDough does not support negative indices or providing step values other than 1.
+- `SLICE`: operator used for string slicing, with the same semantics as Python string slicing. If `s[a:b:c]` is done, that is translated to `SLICE(s,a,b,c)` in PyDough, and any of `a`/`b`/`c` could be absent. Negative slicing is supported, however currently, PyDough does not support providing step values other than 1.


My understanding is this PyDough does not support providing step values other than 1. applies for both positive and negative values.
Current edit makes it that only negative doesn't support step values other than 1.
A suggestion.

Suggested change

- `SLICE`: operator used for string slicing, with the same semantics as Python string slicing. If `s[a:b:c]` is done, that is translated to `SLICE(s,a,b,c)` in PyDough, and any of `a`/`b`/`c` could be absent. Negative slicing is supported, however currently, PyDough does not support providing step values other than 1.

- `SLICE`: operator used for string slicing, with the same semantics as Python string slicing. If `s[a:b:c]` is done, that is translated to `SLICE(s,a,b,c)` in PyDough, and any of `a`/`b`/`c` could be absent. Negative slicing is supported. Currently PyDough does not support providing step values other than 1.

hadia206 · 2025-02-24T18:17:00Z

pydough/sqlglot/transform_bindings.py

    if isinstance(step, sqlglot_expressions.Literal):
-        if int(step.this) != 1:
+        try:
+            step_int = int(step.this)


Shouldn't this be like others to handle None case?
start_idx = int(start.this) if not isinstance(start, sqlglot_expressions.Null) else None

hadia206 · 2025-02-24T18:18:54Z

pydough/sqlglot/transform_bindings.py

+        try:
+            step_int = int(step.this)
+        except ValueError:
+            raise ValueError(


It's better to also add which argument was the issue.
e.g. SLICE function step argument must be integer literal or absent

hadia206 · 2025-02-24T18:19:36Z

pydough/sqlglot/transform_bindings.py

+        )
+    except ValueError:
+        raise ValueError(
+            "SLICE function currently only supports the slicing arguments being integer literals or absent"


Same. Specify argument

hadia206 · 2025-02-24T18:19:53Z

pydough/sqlglot/transform_bindings.py

+        )
+    except ValueError:
+        raise ValueError(
+            "SLICE function currently only supports the slicing arguments being integer literals or absent"


Same. Specify argument

hadia206 · 2025-02-24T18:20:20Z

pydough/sqlglot/transform_bindings.py

+    start_idx: int | None = None
+    stop_idx: int | None = None
+    try:
+        start_idx = (


Why do we have value in ()?

knassre-bodo

Looks fantastic @vineetg3!!! Just a few comments before merging.

knassre-bodo · 2025-02-24T18:23:08Z

documentation/functions.md

+    name_without_first_char = name[1:],
+    last_digit = phone[-1:],
+    name_without_start_and_end_char = name[1:-1]


Lets also add:

phone_without_last_5_chars = phone[:-5]

name_second_to_last_char = name[-2:-1]

knassre-bodo · 2025-02-24T18:27:13Z

pydough/sqlglot/transform_bindings.py

@@ -504,12 +504,68 @@ def convert_concat(
        return Concat(expressions=inputs)


+def positive_index(
+    column: SQLGlotExpression, neg_index: int, is_0_based: bool = False


NIT: is_zero_indexed instead if is_0_based

knassre-bodo · 2025-02-24T18:28:37Z

pydough/sqlglot/transform_bindings.py

+    based on the length of the column.
+
+    Args:
+        `column`: The column expression to reference


NIT: this isn't necessarily a column (could be a function call, or a literal). Let's just call this string_expr and say it refers to the string that the negative index is referencing a point within.

knassre-bodo · 2025-02-24T18:29:28Z

pydough/sqlglot/transform_bindings.py

    """
    assert len(sql_glot_args) == 4
+    _, start, stop, step = sql_glot_args


NIT: can unpack string_expr as the first arg & use that instead of sql_glot_args[0] in the rest of the function.

knassre-bodo · 2025-02-24T18:30:14Z

pydough/sqlglot/transform_bindings.py

    for arg in sql_glot_args[1:]:
        if not isinstance(arg, (sqlglot_expressions.Literal, sqlglot_expressions.Null)):


NIT: can replace this the loop guard with for arg in (start, stop, step)

knassre-bodo · 2025-02-24T18:31:00Z

pydough/sqlglot/transform_bindings.py

+        try:
+            step_int = int(step.this)


NIT: can push this check into the the earlier for loop by checking if isinstance(arg.this, int) when it is a literal.

knassre-bodo · 2025-02-24T18:31:56Z

pydough/sqlglot/transform_bindings.py

+    try:
+        start_idx = (


Same here: no need to check with try/excepts, just psuh the check into the earlier loop & just do start_idx = int(start.this) if start isn't null.

The integer literal is presented as a string by sqlglot, for example 2 is presented as '2'. So, we would need to check if that could be converted to an int by using try catch. This isn't the cleanest way probably and I'm open to other suggestions.

knassre-bodo · 2025-02-24T18:33:10Z

pydough/sqlglot/transform_bindings.py

+            if start_idx < 0 and stop_idx < 0:
+                # Early return if start index is greater than stop index
+                # e.g., "hello"[-2:-4] should return empty string
+                if start_idx > stop_idx:


Should be >= since "hello"[-2:-2] returns an empty string (the upper bound is exclusive.

knassre-bodo · 2025-02-24T18:43:09Z

tests/bad_pydough_functions.py

 def bad_slice_4():
-    # Unsupported slicing: reversed
-    return Customers(name[::-1])
+    # Unsupported slicing: non-integer start
+    return Customers(name[datetime.datetime.now() : -1 :])


What about testing with non-literals?

knassre-bodo · 2025-02-24T18:45:07Z

tests/simple_pydough_functions.py

+
+
+def step_slicing():
+    return Customers.WHERE(name == "Jane Smith")(


Since the test generates the answers, why not just test on the entire table (its only 20 rows)?

vineetg3 added 8 commits February 14, 2025 16:05

[RUN CI] implemented magic methods

0a34a79

[RUN CI] merge conflict

3a331f0

[RUN CI] changes

b8db996

added substr, need to match python

560624d

[RUN CI] support for negative indices in string slicing

24cbbd9

[RUN CI] more tests

efeb739

merge conflict

39f7aef

[RUN CI]

cf28614

vineetg3 requested review from knassre-bodo and hadia206 and removed request for knassre-bodo February 21, 2025 21:23

[RUN CI] better error messages

e6e7be5

hadia206 reviewed Feb 24, 2025

View reviewed changes

knassre-bodo reviewed Feb 24, 2025

View reviewed changes

vineetg3 added 2 commits February 25, 2025 10:48

changes

de513ff

[RUN CI]changes

6f697c3

vineetg3 self-assigned this Feb 26, 2025

vineetg3 requested review from hadia206 and knassre-bodo February 26, 2025 08:06

vineetg3 added 3 commits February 26, 2025 16:45

calcs

cdb2540

[RUN CI] merge changes

65b3677

[RUN CI] merge changes

5938772

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved Slicing to support negative indices #277

Improved Slicing to support negative indices #277

vineetg3 commented Feb 21, 2025

hadia206 left a comment

hadia206 Feb 24, 2025

hadia206 Feb 24, 2025

hadia206 Feb 24, 2025

hadia206 Feb 24, 2025

hadia206 Feb 24, 2025

hadia206 Feb 24, 2025

hadia206 Feb 24, 2025

knassre-bodo left a comment

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

vineetg3 Feb 25, 2025

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

knassre-bodo Feb 24, 2025

	- Convert `stop` & `start` to positive.
	- Convert `stop` to positive.

	- `SLICE`: operator used for string slicing, with the same semantics as Python string slicing. If `s[a:b:c]` is done, that is translated to `SLICE(s,a,b,c)` in PyDough, and any of `a`/`b`/`c` could be absent. Negative slicing is supported, however currently, PyDough does not support providing step values other than 1.
	- `SLICE`: operator used for string slicing, with the same semantics as Python string slicing. If `s[a:b:c]` is done, that is translated to `SLICE(s,a,b,c)` in PyDough, and any of `a`/`b`/`c` could be absent. Negative slicing is supported. Currently PyDough does not support providing step values other than 1.

		for arg in sql_glot_args[1:]:
		if not isinstance(arg, (sqlglot_expressions.Literal, sqlglot_expressions.Null)):



		def step_slicing():
		return Customers.WHERE(name == "Jane Smith")(

Improved Slicing to support negative indices #277

Are you sure you want to change the base?

Improved Slicing to support negative indices #277

Conversation

vineetg3 commented Feb 21, 2025

hadia206 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

knassre-bodo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment