[ZC-1342]: Disable variable syntax in derivations by slobodan-ilic · Pull Request #489 · Crunch-io/scrunch

slobodan-ilic · 2025-08-05T13:34:52Z

All derivation expressions should use {"var": <alias>} syntax. The {"variable": <varid>} syntax is deprecated and will not be supported in the fututre.

jjdelc

This is amazing work!! Thanks @slobodan-ilic

jjdelc · 2025-08-12T14:00:05Z

            'expression': {
                'function': '!=',
                'args': [
-                    {'variable': var.url},  # Crunch needs variable URLs!


Is there somewhere a var variable that isn't used anymore now?

Removed those unused var assignments in the exclusion-filter tests in 50c268c.

jjdelc · 2025-08-12T14:00:17Z

                                "function": "in",
                                "args": [
                                    {
-                                        "variable": var1.url


And a var1 and var2

Yep, removed the unused var1/var2 setup too in 50c268c.

jjdelc · 2025-08-12T14:30:36Z

            # We are in the case of a default fill, replace the -1 with the new
            # variable
-            fill_map["-1"] = {"variable": vars_by_alias[else_case["variable"]]["id"]}
+            fill_map["-1"] = {"var": else_case["var"]}


Is there still a vars_by_alias that's being fetched that we may not need anymore?

I kept vars_by_alias here. We no longer need it to turn fill_map entries into ids, but create_fill_values still uses it to validate source variables and keep the existing scrunch public API where callers pass variable aliases.

Clarifying after the cleanup in 1717fcb: vars_by_alias is no longer used in the fill_map construction. The fill map now uses the scrunch input alias and emits var. The vars_by_alias fetch remains earlier only for the existing categorical type validation.

jjdelc · 2025-08-12T14:34:19Z


-        # inspect function, then inspect variable, if multiple_response,
-        # then change in --> any
-        if 'function' in obj and 'args' in obj:


We're not making this replacement anymore?

This is because in scrunch they say subvar in mr_var but in zz9 it is more like any(mr_var, [subvar]) so they need to replace IN for ANY

Good catch. Restored this path in 50c268c: MR variables parsed with in are normalized through the any handling again, with unit coverage for the var/axes payload.

jjdelc · 2025-08-12T14:36:12Z

+
+            if 'var' in fragment:
+                if 'axes' in fragment:
+                    return "{}[{}]".format(fragment['var'], fragment['axes'][0])


uhhh are we parsing this back? I can't remember if we have tests that show that scrunch can understand this. It's been so long 🧓🏽

Added tests for this. prettify now covers var directly and var+axes as array syntax, and the parser/process path keeps emitting the Crunch API var/axes shape.

More concretely, the prettify coverage is:

TestPrettify.test_var_alias_no_dataset for {"var": ...}

TestPrettify.test_var_square_bracket_subvariables for { "var": ..., "axes": [...] }

TestPrettify.test_transitional_variable_url for the temporary old variable URL response shape

TestPrettify.test_transitional_variable_url_square_bracket_subvariables for old subvariable URL responses

The parser/process outbound var/axes assertions are in:

TestExpressionProcessing.test_process_all_multiple_response

TestExpressionProcessing.test_process_in_multiple_response

jjdelc

The function process_expr is not needed anymore, it exists to convert aliases to URLs, if we're now doing aliases directly. Keeping that function is unevaluated code debt that should also be removed.

With this PR there is no reason to keep all that complex code still around generating confusion.

jjdelc · 2026-06-08T18:30:01Z

I think these docstrings need to change too

Good catch — fixed in 85963c3. Updated the example to use the var term to match what the parser now emits.

slobodan-ilic · 2026-06-10T18:09:43Z

@jjdelc — pushed 85963c3 addressing this. Took a closer look at what process_expr does now, and the alias→URL conversion you mentioned is in fact already gone (removed in earlier commits on this branch, e.g. 796a0c2). What still lives in there is metadata-driven enrichment that parse_expr structurally cannot do without a dataset entity:

subvariable → parent + axes rewrite
category name → id resolution
multiple-response adaptation (column form)
any↔in swap based on array vs categorical
array variable expansion into or(...) over subvariables

Rather than remove these, I refactored for honesty:

Module + process_expr docstrings now describe the five real tasks, state plainly that aliases are no longer converted to URLs, and the top-of-file example uses var to match.
Dropped dead code: base_url, the unused variable_id inner helper, the never-written outer subvariables accumulator.
Extracted the 60-line array-expansion block into _expand_array_filter with a docstring, and replaced the loop-leaking var name with an explicitly tracked array_var.
Section headers inside _process label each phase: walk / subvariable rewrite / any↔in swap / array expansion.

Behavior is unchanged — 337 unit tests still pass. If you can point to specific cases where any of the five rewrites are actually unnecessary against the current API, happy to peel more.

Clarify process_expr instead of removing it. JJ's note assumed the function only translates aliases to URLs; the alias->URL part is in fact already gone (earlier commits on this branch). What remains is metadata-driven enrichment that parse_expr structurally cannot do: subvariable -> parent+axes rewrite, category name -> id resolution, multiple-response adaptation, any<->in swap by variable type, and array variable expansion into or(...) chains. Refactor for honesty: - Module + process_expr docstrings now describe the five real tasks and state plainly that aliases are not converted to URLs. The top-of-file example payload now uses `var` to match. - Drop dead code: base_url, the unused variable_id inner helper, the outer subvariables accumulator that is never written to. - Extract the array-expansion block into _expand_array_filter with a docstring, and replace the loop-leaking `var` name with an explicitly tracked array_var. - Section headers inside _process label each phase: walk, subvariable rewrite, any<->in swap, array expansion. No behavior change — 337 unit tests still pass.

jjdelc

Thanks so much for this work! Great reworking, mustn't been easy to understand all of this to make this improvements. 🚀

jjdelc · 2026-06-11T10:57:27Z

                    folders_by_name[folder_name].entity.delete()
                # Always delete the tmp dataset no matter what
-                tmp_ds.delete()
+                tmp_ds.delete()


Ah, closing newline, good!

jjdelc · 2026-06-11T10:59:27Z

+Expression objects produced by `parse_expr` are purely syntactic: they
+refer to variables by `alias` (in `var` terms) and know nothing about
+the dataset. They are not yet ready to be sent to Crunch -- the API still
+needs some metadata-driven rewrites (subvariable -> parent+axes, category


This is an interesting point, we have something similar happening in automation in which even after having an alias based expression, we still need to do a 2nd pass for the exact same motives.

This makes me think if we should start doing some of this in the API instead of demanding consumers (Scrunch, Crunch Automation) having to deal with this as well.

jjdelc · 2026-06-11T11:00:28Z

    return list(range(lower, upper + 1))


-def parse_expr(expr, platonic=False):


jjdelc · 2026-06-11T11:04:49Z

+
+    Aliases are NOT converted to URLs -- the API accepts alias-based `var`
+    terms directly. Alias existence is validated here as a side-effect of
+    the metadata lookups.


Great docstring! Thanks for the clarity!

slobodan-ilic requested a review from jjdelc August 5, 2025 16:39

slobodan-ilic force-pushed the ZC-1342-disable-variable-syntax-in-derivations branch from c4633ae to 25bcb3d Compare August 8, 2025 09:14

jjdelc reviewed Aug 12, 2025

View reviewed changes

remove "variable" syntax for expressions

796a0c2

slobodan-ilic force-pushed the ZC-1342-disable-variable-syntax-in-derivations branch from 72263e3 to 796a0c2 Compare June 5, 2026 15:42

slobodan-ilic added 4 commits June 5, 2026 20:02

Preserve scrunch API while emitting var derivations

ecf7b6b

Address review comments for var expressions

50c268c

Make multiple response expansion deterministic

88191cb

Keep fill values input on variable syntax

1717fcb

jjdelc requested changes Jun 8, 2026

View reviewed changes

slobodan-ilic force-pushed the ZC-1342-disable-variable-syntax-in-derivations branch 4 times, most recently from 3d9d1d9 to 85963c3 Compare June 10, 2026 18:09

slobodan-ilic force-pushed the ZC-1342-disable-variable-syntax-in-derivations branch from 85963c3 to d5fc1ea Compare June 10, 2026 18:18

jjdelc approved these changes Jun 11, 2026

View reviewed changes

Bump version to 0.18.22

059cb55

slobodan-ilic merged commit c7dc749 into master Jun 12, 2026
5 checks passed

		return list(range(lower, upper + 1))


		def parse_expr(expr, platonic=False):

Uh oh!

Conversation

slobodan-ilic commented Aug 5, 2025

Uh oh!

jjdelc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slobodan-ilic Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jjdelc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slobodan-ilic commented Jun 10, 2026

Uh oh!

jjdelc left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

slobodan-ilic Jun 5, 2026 •

edited

Loading