Support several variables #1056
Conversation
|
I have to fix tests! |
There was a problem hiding this comment.
Only some details to be tackled or not, as you wish.
But what we need to add are tests to ensure the final queries with only one table and variable, only a table and several variables, and several tables and several variables. And putting the green light in tests.
|
|
||
| for variable in variables: | ||
| variable_split = variable.split('.') | ||
| table, variable = variable_split[-2], variable_split[-1] | ||
| project_part, dataset_part, table_part, variable_part = variable_split |
There was a problem hiding this comment.
I would use project, dataset, table, variable
There was a problem hiding this comment.
Sure, better naming!
| @@ -22,9 +24,9 @@ def enrich(query_function, **kwargs): | |||
| data_copy = _prepare_data(kwargs['data'], kwargs['data_geom_column']) | |||
| tablename = _upload_dataframe(bq_client, user_dataset, data_copy, kwargs['data_geom_column']) | |||
|
|
|||
| query = _enrichment_query(user_dataset, tablename, query_function, **kwargs) | |||
| queries = _enrichment_query(user_dataset, tablename, query_function, **kwargs) | |||
There was a problem hiding this comment.
I would rename method to: _enrichment_queries
There was a problem hiding this comment.
You are right!
|
@oleurud Tests fixed, they are failing because of another E2E test. |
| query_function = _prepare_sql_by_points | ||
| variables = pd.DataFrame([['table1.var1'], ['table1.var2']], columns=['id']) | ||
| variables = pd.DataFrame([['project.dataset.category_table1_comp_geog_geogyear_yearly_datayear.var1'], |
There was a problem hiding this comment.
I see we are modifying the previous tests, but as @oleurud suggested we should have tests for both cases:
- One for multiple variables from one dataset
- Another one for multiple variables from several datasets
There was a problem hiding this comment.
Yes we should. Based on this one, you can add both cases easily
test/data/enrichment/test_service.py
Outdated
| for query in queries] | ||
|
|
||
| self.assertEqual(sorted(queries), sorted(expected_queries)) | ||
|
|
|
Can we merge this one? |
|
@Jesus89 Wait, while doing acceptance I've found out the notebook is not up to date. @alejandrohall could you sync with develop and make it work? Thanks! |
|
Acceptance 🍏 |
No description provided.