Add additional functions to PyDough #197

knassre-bodo · 2025-01-09T18:46:41Z

Adds the following functions: PRESENT, ABSENT, KEEP_IF, ROUND, JOIN_STRINGS, as well implementing SQLGlot conversion support for string slicing. Part of the larger effort for #156.

…upport in PyDough with SQLGlot conversion [RUN CI]

pydough/sqlglot/transform_bindings.py

knassre-bodo · 2025-01-09T20:51:25Z

pydough/sqlglot/transform_bindings.py

+        if int(step.this) != 1:
+            raise NotImplementedError(
+                "SLICE function currently only supports a step of 1"
+            )


For now, does not support reverse order or skipping

Just curious, why?

Because those can't be supported with the basic SQL SUBSTRING function (which is ANSI).

For more dialect-specific bindings we can support some other cases. E.g. s[-n:] becomes RIGHT(s, n) and s[a:b:-1], but only for some dialects.

What is the goal of the slice functionality? Match Python builtin? Numpy indexing?

Python builtin (e.g. Customers(prefix=name[:2] gets the first 2 characters of every customer's name).

knassre-bodo · 2025-01-09T20:51:37Z

pydough/sqlglot/transform_bindings.py

+        if int(start.this) < 0:
+            raise NotImplementedError(
+                "SLICE function currently only supports non-negative start indices"
+            )
+        start_idx = int(start.this)
+    if isinstance(stop, sqlglot_expressions.Literal):
+        if int(stop.this) < 0:
+            raise NotImplementedError(
+                "SLICE function currently only supports non-negative stop indices"
+            )


For now, does not support negative indexing

hadia206

Overall code makes sense and doing is what it's saying.
Most of my comments are questions and minor suggestions.

pydough/pydough_operators/__init__.py

hadia206 · 2025-01-09T21:04:57Z

pydough/pydough_operators/expression_operators/registered_expression_operators.py

+    "KEEP_IF", False, RequireNumArgs(2), SelectArgumentType(0)
+)
+JOIN_STRINGS = ExpressionFunctionOperator(
+    "JOIN_STRINGS", False, RequireMinArgs(1), ConstantType(StringType())


Does this accept only 1 arg the delimiter only?

It requires at least one argument (the class has a docstring)

pydough/sqlglot/transform_bindings.py

hadia206 · 2025-01-09T21:10:35Z

pydough/sqlglot/transform_bindings.py

+    Returns:
+        The SQLGlot case expression equivalent to the `KEEP_IF` call.
+    """
+    return sqlglot_expressions.Case().when(sql_glot_args[1], sql_glot_args[0])


Is the else condition (i.e. null value) case happens automatically.

Yes. You don't need the else in SQL. You can just write CASE WHEN A THEN B END (its implied that there is a ELSE NULL).

@knassre-bodo Can you feed this into convert_iff_case instead? If we want to avoid case statements that seems like the most sustainable way.

Wdym by avoid case statements? convert_iff_case will always create one, it's just that the bindings will sometimes toggle that function on/off.

pydough/sqlglot/transform_bindings.py

hadia206 · 2025-01-09T21:18:58Z

pydough/sqlglot/transform_bindings.py

+        if int(step.this) != 1:
+            raise NotImplementedError(
+                "SLICE function currently only supports a step of 1"
+            )


Just curious, why?

hadia206 · 2025-01-09T21:19:41Z

pydough/sqlglot/transform_bindings.py

+        case (_, None):
+            assert start_idx is not None
+            return sqlglot_expressions.Substring(
+                this=sql_glot_args[0], start=sqlglot_expressions.convert(start_idx + 1)


why start_idx + 1 and not just start_idx?

Because SUBSTRING in SQL is 1-indexed, not 0-indexed.

hadia206 · 2025-01-09T21:20:30Z

pydough/sqlglot/transform_bindings.py

+        case (None, None):
+            raise sql_glot_args[0]
+        case (_, None):
+            assert start_idx is not None


Is this needed? I thought case (None, None) will handle that?

The assertion is so mypy can infer that it is non-null (apparently the pattern matching isn't fully sufficient for that).

pydough/sqlglot/transform_bindings.py

tests/simple_pydough_functions.py

njriasan

Thanks @knassre-bodo! See my suggestions.

njriasan · 2025-01-10T15:42:08Z

pydough/pydough_operators/expression_operators/README.md

@@ -79,10 +86,15 @@ These functions must be called on singular data as a function.

 - `IFF`: if the first argument is true returns the second argument, otherwise returns the third argument.
 - `DEFAULT_TO`: returns the first of its arguments that is non-null.
+- `PRESENT`: returns True if the argument is non-null.


We should open a backlog ticket to see if we want to "standardize" any of these names before open source publication. Calling this PRESENT instead of IS_NOT_NULL may not be useful and may just increase the learning barrier.

Will open that issue. I'm going for human readable names that sound closer to how the question will be phrased (to help the LLM), e.g.:

How many people don't have a middle name (absent)

What is the percentage of patients at the hospital have been diagnosed (present)

How much money is saved by users in each region if all taxes are removed when the shipping date coincides with a holiday (keep_if)

njriasan · 2025-01-10T15:43:04Z

pydough/relational/relational_nodes/column_pruner.py

+        # (e.g. a COUNT(*)), arbitrarily mark one of them as used.
+        # TODO: (gh #196) optimize this functionality so it doesn't keep an
+        # unnecessary column.
+        if isinstance(node, Aggregate) and len(found_identifiers) == 0:


Is this from the other PR? If so maybe we can merge main back into this PR.

Yeah it is, will re-merge.

njriasan · 2025-01-10T15:45:55Z

pydough/sqlglot/transform_bindings.py

@@ -37,7 +37,7 @@ def apply_parens(expression: SQLGlotExpression) -> SQLGlotExpression:
    Returns:
        The expression, wrapped in parentheses if necessary.
    """
-    if isinstance(expression, (Binary, Concat)):
+    if isinstance(expression, (Binary, Concat, Is, Case)):


For sustainability maybe we should make this a global variable set/tuple and then this could become isinstance(expression, PAREN_EXPRESSIONS). Not necessary at this stage, but just a thought in case its ever reused. Feel free to ignore this suggestion.

No no... I think this makes sense.

njriasan · 2025-01-10T15:47:01Z

pydough/sqlglot/transform_bindings.py

+    Returns:
+        The SQLGlot case expression equivalent to the `KEEP_IF` call.
+    """
+    return sqlglot_expressions.Case().when(sql_glot_args[1], sql_glot_args[0])


@knassre-bodo Can you feed this into convert_iff_case instead? If we want to avoid case statements that seems like the most sustainable way.

njriasan · 2025-01-10T15:48:17Z

pydough/sqlglot/transform_bindings.py

+        if int(step.this) != 1:
+            raise NotImplementedError(
+                "SLICE function currently only supports a step of 1"
+            )


What is the goal of the slice functionality? Match Python builtin? Numpy indexing?

pydough/sqlglot/transform_bindings.py

knassre-bodo added 4 commits January 9, 2025 10:26

Suitable fix for COUNT(*) bug [RUN CI]

497bd5e

Adding issue to comment

9305f3c

[RUN CI]

600ddf6

Adding PRESENT, ABSENT, KEEP_IF, MONOTONIC, ROUND, and JOIN_STRINGS s…

debf15a

…upport in PyDough with SQLGlot conversion [RUN CI]

knassre-bodo changed the base branch from main to kian/count_star_bug January 9, 2025 18:46

knassre-bodo requested review from njriasan and hadia206 January 9, 2025 20:09

knassre-bodo added 3 commits January 9, 2025 15:12

Fixing plan test [RUN CI]

d4e1c25

Merge branch 'kian/count_star_bug' into kian/mandatory_functions

aa16d9e

SQLite version handling for concat_ws [RUN CI]

13a18e2

knassre-bodo commented Jan 9, 2025

View reviewed changes

pydough/sqlglot/transform_bindings.py Show resolved Hide resolved

knassre-bodo commented Jan 9, 2025

View reviewed changes

hadia206 approved these changes Jan 9, 2025

View reviewed changes

Base automatically changed from kian/count_star_bug to main January 10, 2025 14:24

knassre-bodo added 2 commits January 10, 2025 09:45

Added error handling tests [RUN CI]

8e098bf

Resolving conflicts [RUN CI]

26815fd

njriasan approved these changes Jan 10, 2025

View reviewed changes

Revisions [RUN CI]

dd88675

knassre-bodo merged commit d1a796f into main Jan 10, 2025
4 checks passed

knassre-bodo deleted the kian/mandatory_functions branch January 10, 2025 20:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add additional functions to PyDough #197

Add additional functions to PyDough #197

knassre-bodo commented Jan 9, 2025

knassre-bodo Jan 9, 2025

hadia206 Jan 9, 2025

knassre-bodo Jan 10, 2025 •

edited

Loading

njriasan Jan 10, 2025

knassre-bodo Jan 10, 2025 •

edited

Loading

knassre-bodo Jan 9, 2025

hadia206 left a comment

hadia206 Jan 9, 2025

knassre-bodo Jan 10, 2025

hadia206 Jan 9, 2025

knassre-bodo Jan 10, 2025

njriasan Jan 10, 2025

knassre-bodo Jan 10, 2025 •

edited

Loading

hadia206 Jan 9, 2025

hadia206 Jan 9, 2025

knassre-bodo Jan 10, 2025

hadia206 Jan 9, 2025

knassre-bodo Jan 10, 2025

njriasan left a comment

njriasan Jan 10, 2025

knassre-bodo Jan 10, 2025

knassre-bodo Jan 10, 2025

njriasan Jan 10, 2025

knassre-bodo Jan 10, 2025

njriasan Jan 10, 2025

knassre-bodo Jan 10, 2025

njriasan Jan 10, 2025

njriasan Jan 10, 2025

Add additional functions to PyDough #197

Add additional functions to PyDough #197

Conversation

knassre-bodo commented Jan 9, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knassre-bodo Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knassre-bodo Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

knassre-bodo Jan 10, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njriasan 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

knassre-bodo Jan 10, 2025 •

edited

Loading

knassre-bodo Jan 10, 2025 •

edited

Loading

knassre-bodo Jan 10, 2025 •

edited

Loading