-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block taking jinja2.runtime.Undefined into DatabricksAdapter #98
Conversation
If we call |
For example,
Also updated the description. |
if "database" not in data["path"]: | ||
data["path"]["database"] = None | ||
else: | ||
data["path"]["database"] = remove_undefined(data["path"]["database"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am curious when will "database" here be Undefined? Is there a specific configuration that can trigger this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While parsing profiles, models, etc. and building metadata that happen before setting up DatabricksRelation
, it could contain Undefined
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also macros for snapshot seems to cause the issue after we change DatabricksRelation.include_policy.database=True
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think it might be helpful to add a test case here (if we set DatabricksRelation.include_policy.database=True
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what kind of test in your mind?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For example, construct a test with the database field being jinja Undefined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a test. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Thanks for adding the test.
Thanks! merging. |
…101) ### Description Backport of #98 Blocks taking `jinja2.runtime.Undefined` into `DatabricksAdapter`. When calling the adapter method from macros, `jinja2.runtime.Undefined` can be passed to `DatabricksAdapter` that could cause unexpected behavior. For example, https://github.com/databricks/dbt-databricks/blob/b8152b697b003e02fb52e86f59b2bde4b492cc0a/dbt/include/databricks/macros/materializations/snapshot.sql#L33 The `model.database` is not defined and `jinja2.runtime.Undefined` will be used to call `adapter.check_schema_exists` and `Cursor.schema()` will fail: ``` [debug] [Thread-13 ]: Databricks adapter: Error while running: GetSchemas(database=, schema=test16533281379208408401_test_basic) [debug] [Thread-13 ]: Databricks adapter: <class 'databricks.sql.exc.RequestError'>: Error during request to server [debug] [Thread-13 ]: finished collecting timing info [debug] [Thread-13 ]: On snapshot.snapshot_strategy_timestamp.ts_snapshot: Close [error] [Thread-13 ]: Unhandled error while executing snapshot.snapshot_strategy_timestamp.ts_snapshot Error during request to server Unhandled error while executing snapshot.snapshot_strategy_timestamp.ts_snapshot Error during request to server ```
) ### Description Backport of #98 Blocks taking `jinja2.runtime.Undefined` into `DatabricksAdapter`. When calling the adapter method from macros, `jinja2.runtime.Undefined` can be passed to `DatabricksAdapter` that could cause unexpected behavior. For example, https://github.com/databricks/dbt-databricks/blob/b8152b697b003e02fb52e86f59b2bde4b492cc0a/dbt/include/databricks/macros/materializations/snapshot.sql#L33 The `model.database` is not defined and `jinja2.runtime.Undefined` will be used to call `adapter.check_schema_exists` and `Cursor.schema()` will fail: ``` [debug] [Thread-13 ]: Databricks adapter: Error while running: GetSchemas(database=, schema=test16533281379208408401_test_basic) [debug] [Thread-13 ]: Databricks adapter: <class 'databricks.sql.exc.RequestError'>: Error during request to server [debug] [Thread-13 ]: finished collecting timing info [debug] [Thread-13 ]: On snapshot.snapshot_strategy_timestamp.ts_snapshot: Close [error] [Thread-13 ]: Unhandled error while executing snapshot.snapshot_strategy_timestamp.ts_snapshot Error during request to server Unhandled error while executing snapshot.snapshot_strategy_timestamp.ts_snapshot Error during request to server ```
Description
Blocks taking
jinja2.runtime.Undefined
intoDatabricksAdapter
.When calling the adapter method from macros,
jinja2.runtime.Undefined
can be passed toDatabricksAdapter
that could cause unexpected behavior.For example,
dbt-databricks/dbt/include/databricks/macros/materializations/snapshot.sql
Line 33 in b8152b6
The
model.database
is not defined andjinja2.runtime.Undefined
will be used to calladapter.check_schema_exists
andCursor.schema()
will fail: