Conversation
nishantmonu51
left a comment
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
DSNs are set but the accounts are inactive. Same convention is being followed in all the test cases.
It is okay to fix it correctly later. No need for release note item since measure filters with unnest cols are rare. |
unnestdid not work for snowflake and bigquery.Follow ups:
Checklist: