feat(bigquery): use native UNPIVOT for pivot_longer - #12091
dlstadther wants to merge 7 commits into
Conversation
BigQuery's UNPIVOT is significantly cheaper than the generic struct-packing/array/unnest lowering pivot_longer uses by default. Introduce ops.PivotLonger as a proper relational op (schema and the generic lowering both derive from a single to_generic() implementation, so every other backend is unaffected) and give BigQuery a conditional LOWERED_OPS rewrite that renders UNPIVOT INCLUDE NULLS when there's a single names_to column, falling back to the generic path otherwise (UNPIVOT only ever produces one name column). Two correctness gaps surfaced via a live BigQuery smoke test and got fixed: BigQuery's UNPIVOT IN-list requires exact type equality across columns (unlike ibis's own array/struct unification), so each pivoted column is cast to the resolved value type first; and UNPIVOT's physical column order (value_col, name_col) doesn't match ibis's declared schema order (name_col, value_col), which silently mislabeled results, so the outer select now lists columns explicitly in schema order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWzsGWfxosFyz9abPziykS
…CLUDE NULLS pivot_longer had no way to drop rows where the pivoted value is NULL short of a separate .drop_null() call after the fact. Add values_drop_na (named after tidyr's pivot_longer argument of the same purpose), applied in PivotLonger.to_generic() so every backend gets it via the existing generic lowering, and mapped to BigQuery's native UNPIVOT EXCLUDE NULLS instead of a post-filter when the native path is taken. Default is False, matching the existing always-keep-NULL-rows behavior. Verified against real BigQuery: values_drop_na=True drops the NULL row via EXCLUDE NULLS, values_drop_na=False (default) still uses INCLUDE NULLS unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWzsGWfxosFyz9abPziykS
…lt for pivot_longer Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWzsGWfxosFyz9abPziykS
… INCLUDE NULLS Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWzsGWfxosFyz9abPziykS
Polars uses its own singledispatch `translate` compiler, not SQLGlotCompiler's `LOWERED_OPS` lowering, so the new `PivotLonger` relational op reached it untranslated and raised `OperationNotDefinedError`. Register a translation that falls back to `to_generic()`, same as every other non-BigQuery backend. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PWzsGWfxosFyz9abPziykS
|
This looks reasonable at a high level. Can you do me a favor and try and check that the added code is idiomatic and aligned in style with the rest of the repo? Some of the AI-coding things stick out even at a glance, like the massive comments and notes (which you won't see throughout the rest of the repo); haven't looked deeply, but this will help reduce some churn. Other question that will come up with is, which over backends could benefit from switching to a native UNPIVOT? I imagine it's not just BigQuery; haven't looked, and not asking you to change other backends in this PR (if anything, maybe this PR scoped to BigQuery will be easier to review), but it will be helpful to have an answer for which backends adding the native op could potentially benefit. I'll try to get to reviewing this properly once these changes are in, but preemptively forgive the fact that there will probably be some delay. |
|
Thanks for your quick initial response @deepyaman . I'll go back through this proposal and ensure it aligns in style with rest of Ibis. I'll also trim unnecessary comments at the same time. As for scope of changes and other areas that could benefit from UNPIVOT, I suspect there are other candidates, but I wish to keep this PR scoped to what is minimally required to support the backend override + BigQuery + address failing tests (which is why there are adjustments to polars here). |
Comments and docstrings around the PivotLonger UNPIVOT lowering were much longer and more note-like than the rest of the codebase's style. Trim them to match the terse, why-only comment convention used elsewhere (e.g. visit_TableUnnest, translate.register functions). Addresses review feedback from @deepyaman on PR ibis-project#12091.
…h repo conventions - Rename lower_pivot_longer_bigquery -> pivot_longer_to_unpivot. No other backend-specific @replace rewrite in this codebase uses the "lower_" prefix (reserved for shared rewrites.py/base.py factories) or a redundant backend-name suffix; local ones are named for what they do (offset_to_filter, rewrite_rows_range_order_by_window, etc). - Replace `import ibis` + `ibis.struct(...)`/`ibis.array(...)` in PivotLonger.to_generic() with `from ibis.expr.types import array, struct`. A top-level import isn't possible here (confirmed: ibis.expr.api pulls in ibis.expr.types -> ibis.expr.builders -> ops before ops finishes loading, a circular import); the deferred import stays, narrowed to the specific names' actual home, matching the same pattern already used in ops/core.py. Continues addressing review feedback from @deepyaman on PR ibis-project#12091.
|
Went back through and trimmed the AI-generated bloat:
Left Still scoped to BigQuery only per the earlier discussion; the "which other backends could benefit" question is open for whenever you get to a full review. I have not checked on usefulness for other backends yet. Though, i still would NOT advocate to expand the scope of this PR beyond BQ + necessary for tests/compatibility. |
|
Hi @deepyaman , checking in to see if you (or any other maintainer) have opportunity to review |
|
Sorry, it's on my list, but honestly I will try to get to it next week
(doubtful this week, due to work travel).
…On Thu, Sep 17, 2026, 3:00 PM Dillon Stadther ***@***.***> wrote:
*dlstadther* left a comment (ibis-project/ibis#12091)
<#12091 (comment)>
Hi @deepyaman <https://github.com/deepyaman> , checking in to see if you
(or any other maintainer) have opportunity to review
—
Reply to this email directly, view it on GitHub
<#12091?email_source=notifications&email_token=ADK3W3QGH2D3XI3E47AUODL5PQYFTA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRHE3DSMRRHAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5719692181>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADK3W3VLJRAFKFZPPUMUDZD5PQYFTAVCNFSNUABEKJSXA33TNF2G64TZHMZTIMJTHEZDGMB3JFZXG5LFHM2TENZSGI2DEMJTGSQXMAQ>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/ADK3W3WSAR3DSGMMZMHWBBD5PQYFTA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRHE3DSMRRHAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/ADK3W3RW76PRINPVC3W34TL5PQYFTA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKNZRHE3DSMRRHAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Description of changes
Table.pivot_longercurrently compiles to a generic struct-pack / array / unnest expression on every backend, including BigQuery, even though BigQuery has a nativeUNPIVOToperator for exactly this shape of query. This PR adds a BigQuery-specific compilation path that usesUNPIVOTwhen possible, matching the existing pattern of backend-specific rewrites viaLOWERED_OPS.ops.PivotLongeris now a real relational op (instead of being eagerly expanded into struct/array/unnest at expression-build time), so a backend can special-case it during compilation.to_generic()on the op still produces the original struct/array/unnest tree and remains the implementation for every other backend.pivot_longercall with a singlenames_tocolumn compiles to a nativeUNPIVOT. A call with more than onenames_tocolumn (e.g. from a multi-groupnames_pattern) still falls back to the generic implementation, sinceUNPIVOT(in both its single-column and multi-column forms) only ever produces a single name column — it can't express multiplenames_tocolumns.UNPIVOT'sIN (...)list requires every column to share the exact same type (no implicit widening), so pivot columns of different-but-unifiable types (e.g.int64+float64) are now explicitly cast to the pivoted column's unified type before entering theINlist.UNPIVOT's physical output column order is always[...passthrough, value_col, name_col], which does not matchPivotLonger's declared schema order of[...passthrough, name_col, value_col]. Since results are mapped to schema positionally, this silently swapped the name/value columns' data. The generated SQL now selects columns explicitly in schema order rather than relying onSELECT *.values_drop_na: bool = Falsetopivot_longer. WhenTrue, rows where the resultingvalues_tocolumn isNULLare dropped (result.drop_null(values_to)in the generic implementation). This is implemented once into_generic(), so every backend gets it for free. On BigQuery it maps toUNPIVOT's nativeEXCLUDE NULLSmodifier instead.pivot_longerkeepsNULLs by default (values_drop_na=False) on every backend, which is the opposite ofUNPIVOT's own default (EXCLUDE NULLS) — the generated BigQuery SQL always statesINCLUDE NULLSorEXCLUDE NULLSexplicitly, so it never silently relies on BigQuery's default.All of the above (native
UNPIVOTcompilation, the type-cast fix, the column-order fix, andvalues_drop_na) were also verified against a live BigQuery project, comparing results row-for-row againstpandas.melt(), in addition to the unit tests included here.This is a drop-in replacement: output is identical to the previous struct/array/unnest implementation for BigQuery users, and there is no behavior change for any other backend.
This addresses an active query performance issue I'm hitting in production use of Ibis against BigQuery. AI (Claude) was used to aid in scoping and implementing these changes, including the live BigQuery testing referenced above.