Skip to content

fix: improvements and fixes to unnest support - #9883

Open
k-anshul wants to merge 9 commits into
mainfrom
unnest_fix_bq_snowflake
Open

k-anshul wants to merge 9 commits into
mainfrom
unnest_fix_bq_snowflake

Conversation

@k-anshul

@k-anshul k-anshul commented Sep 14, 2026

Copy link
Copy Markdown
Member
  1. unnest did not work for snowflake and bigquery.
  2. Filter on unnest exploded the list for databricks.

Follow ups:

  1. Subquery with unnest does not work on main.

Checklist:

  • Covered by tests
  • Ran it and it works as intended
  • Reviewed the diff before requesting a review
  • Checked for unhandled edge cases
  • Linked the issues it closes
  • Checked if the docs need to be updated. If so, create a separate Linear DOCS issue
  • Intend to cherry-pick into the release branch
  • I'm proud of this work!

@k-anshul k-anshul self-assigned this Sep 14, 2026
@k-anshul k-anshul changed the title fix: missing unnext support for bigquery and snowflake: fix: missing unnest support for bigquery and snowflake: Sep 15, 2026
@k-anshul k-anshul changed the title fix: missing unnest support for bigquery and snowflake: fix: improvements and fixes to unnest support Sep 16, 2026
@nishantmonu51 nishantmonu51 added Type:Bug Something isn't working Area:Connectors Size:L Large change: 500-1,999 lines labels Sep 16, 2026

@nishantmonu51 nishantmonu51 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One note that does not anchor to a changed line. The Databricks behaviour change is worth a release-note item: a Subquery filter on an unnest dimension that is not selected — what the explore UI generates for measure-based dimension filters — now fails with the right value must be a list of values for an array IN condition (runtime/metricsview/astexpr.go:684) instead of running. It previously ran, but through the row-exploding LATERAL VIEW join this PR removes, so the numbers were double-counted. Turning silently wrong results into an error is the right trade, but Databricks users will experience it as a regression until the subquery follow-up lands.


// TestUnnestDimension creates a table in the DSN's current database and schema, so the DSN must point at a writable schema.
func TestUnnestDimension(t *testing.T) {
t.Skip("skipping due to inactive Snowflake account")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The unconditional skip means none of the new Snowflake SQL is exercised against a real database — not in this PR's checks, and not after the account is reactivated, since nothing about reactivation removes a hard-coded t.Skip. That leaves the LATERAL FLATTEN(...) t0 (seq, key, path, index, tags, this) column-alias form and the FILTER(..., t0 -> ...) lambda with bound ? parameters verified only by local runs. testruntime.AcquireConnector does not skip on missing credentials on its own — it fails via require.NotEmpty (runtime/testruntime/connectors.go:80) — so skipping on an empty RILL_RUNTIME_SNOWFLAKE_TEST_DSN here would let the test come back by itself.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSNs are set but the accounts are inactive. Same convention is being followed in all the test cases.

}

func TestUnnestDimension(t *testing.T) {
t.Skip("skipping due to inactive Databricks account")

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the Snowflake case: the hard-coded skip leaves the EXISTS(arr, t0 -> ...) lambda, the arrays_overlap path and the switch to tupleStyle: true in LateralUnnest unverified against a real warehouse, and it will outlive the inactive account. Gating on an empty RILL_RUNTIME_DATABRICKS_TEST_DSN instead of an unconditional skip would restore coverage automatically.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSNs are set but the accounts are inactive. Same convention is being followed in all the test cases.

@k-anshul

Copy link
Copy Markdown
Member Author

One note that does not anchor to a changed line. The Databricks behaviour change is worth a release-note item: a Subquery filter on an unnest dimension that is not selected — what the explore UI generates for measure-based dimension filters — now fails with the right value must be a list of values for an array IN condition (runtime/metricsview/astexpr.go:684) instead of running. It previously ran, but through the row-exploding LATERAL VIEW join this PR removes, so the numbers were double-counted. Turning silently wrong results into an error is the right trade, but Databricks users will experience it as a regression until the subquery follow-up lands.

It is okay to fix it correctly later. No need for release note item since measure filters with unnest cols are rare.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area:Connectors Size:L Large change: 500-1,999 lines Type:Bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants