draft updates - #1
Conversation
denimalpaca
left a comment
denimalpaca
left a comment
There was a problem hiding this comment.
Have some initial comments and questions, but overall I like the way this is getting dialed-in a lot!
| @@ -38,8 +38,6 @@ class GreatExpectationsOperator(BaseOperator): | |||
| :type file_regex: Optional[Dict] | |||
| runtime_data_source: Optional[Union[DataFrame, str]] = None, | ||
| dataframe_to_validate: Optional[DataFrame] = None, # should we allow a Spark DataFrame as well? | ||
| query_to_validate: Optional[str] = None, |
There was a problem hiding this comment.
Remove and add docstring lines as necessary
| self.return_json_dict: bool = return_json_dict | ||
| self.use_open_lineage = use_open_lineage | ||
|
|
||
| if self.dataframe_to_validate is not None and self.query_to_validate: |
There was a problem hiding this comment.
Want to check if self.query_to_validate is not None for consistency?
| raise ValueError( | ||
| "Exactly one, or neither, of dataframe_to_validate or query_to_validate may be specified." | ||
| ) | ||
| self.runtime_datasource = bool(self.dataframe_to_validate is not None) or bool(self.query_to_validate) |
There was a problem hiding this comment.
Same as above, consistency in checks (unless I'm missing something).
| self.runtime_datasource = bool(self.dataframe_to_validate is not None) or bool(self.query_to_validate) | ||
| # Check that only one of the arguments is passed to set a data context | ||
| if not (self.data_context_root_dir ^ self.data_context_config): | ||
| if not (bool(self.data_context_root_dir) ^ bool(self.data_context_config)): |
There was a problem hiding this comment.
Not sure why the bools are added here (and not below as well).
| "default_regex": self.file_regex, | ||
| }, | ||
| } | ||
| pass |
There was a problem hiding this comment.
Maybe this should throw and error - something about not recognizing the data source and needing further configuration?
| # BigQuery needs a temp table to run on; it is assumed the table will | ||
| # be named the same as the data asset but with an added _temp suffix | ||
| if self.conn_type == "gcpbigquery": | ||
| self.batch_request_extra["batch_spec_passthrough"] = { | ||
| "bigquery_temp_table": f"{self.data_asset_name}_temp" | ||
| } | ||
| batch_request.update(self.batch_request_extra) |
There was a problem hiding this comment.
Is this no longer needed for BigQuery? I can't tell if it's being handled somewhere else.
| else: | ||
| self.log.info("Validation with Great Expectations successful.") | ||
|
|
||
| def build_configured_sql_datasource_config_from_conn_id(self): |
There was a problem hiding this comment.
Is this and the method below going to be called anywhere? I don't quite understand how they fit into the process.
2 participants
A few updates here. Some open questions:
I think it makes sense to get rid of the default filesystem datasources, though it still might make sense for dataframes, since there is basically no configuration required here.
Need to differentiate query from dataframe with Runtime Connectors, and pandas from spark df.
How much Data Context config is fair to require as prerequisite for the project? I think what you have here is probably fine (requiring a basic DataContext which will have Stores), but it would be great to think this through.