DataStore Records Return (Core PR#8684)#200
DataStore Records Return (Core PR#8684)#200JVickery-TBS wants to merge 3 commits intocanada-v2.10from
Conversation
- Return records capability. - Improved ds upsert sql.
- Added change log file.
| return_columns=return_columns) if data_dict[ | ||
| 'include_records'] else '', | ||
| values=('%s, ' % pk_values_sql if pk_sql == '"_id"' else '') + | ||
| ', '.join([f'cast(:{p} as nested)' if field['type'] == 'nested' |
There was a problem hiding this comment.
this nested type haunts the datastore code base since before postgres had a built-in json type 👻
There was a problem hiding this comment.
r we fixing it upstream eventually?
| # (canada fork only): https://github.com/ckan/ckan/pull/8684 | ||
| results =_execute_single_statement(context, sql_string, where_values) | ||
| if data_dict['include_records']: | ||
| data_dict['deleted_records'] = [dict(r) for r in results.mappings().all()] |
There was a problem hiding this comment.
I could see this causing a MemoryError when deleting all the rows in a large table.
For dsaudit we only need the first N rows to populate the activity. Can we apply a LIMIT to a DELETE ... RETURNING?
There was a problem hiding this comment.
you have to do something like this I guess: https://stackoverflow.com/a/37043809
which would require some bigger sql statement changes which could be done. This code is for my datastore solr index plugin, so I would need to have all of the deleted records I think?
| result.pop('connection_url', None) | ||
| # (canada fork only): https://github.com/ckan/ckan/pull/8684 | ||
| if not data_dict.pop('include_records', False): | ||
| result.pop('deleted_records', None) |
There was a problem hiding this comment.
for all of the other endpoints "include_records" adds a "records" value to the return but datastore_delete is different?
There was a problem hiding this comment.
yeah, just semantically for API users to make it clear that the records were deleted
- Upstream PR changes.
2 participants
feat(dev): ds return backport;
ckan#8684