Fix column names using df in upload #964
Conversation
| @@ -89,10 +86,27 @@ def _copyfrom(self, normalized_column_names, with_lnglat): | |||
| columns_origin, | |||
| with_lnglat, | |||
| geom_col, | |||
| enc_type) | |||
| enc_type, | |||
| len(columns_normalized)) | |||
There was a problem hiding this comment.
Why is this length needed now? Oh, is it because with lnglat columns_origin would have a column less than normalized columns, which has the_geom?
There was a problem hiding this comment.
Right now, how we get the columns is a bit weird, basically, because we have the responsibility spread in several methods. With that, I want to ensure the same number of columns in the COPY query and in the _rows method. And yes, it smells.
Case 1: _rows is receiving:
- cols: the column names, but without the geometry one
- geom_col: the geom one or None
- with_lnglat: to generate geom column by latitude and longitude columns
We could create 2 geom columns (if geom_col and with_lnglat are set). The number of column allow us to avoid that.
Case 2: filtering column names:
We are filtering column names, getting the geom in different places, getting columns in different places (table creation, copy query, _rows), ... As a result, we could get a different number of columns in different places. This is the real source of the bad smell
There was a problem hiding this comment.
Apart from the fix I think it's an improvement to have moved some stuff to _copyfrom_column_names (and it allowed for the simple tests). I find it still confusing that origin/normalized columns business with columns in different orders etc. (it took me a while to understand and accept it was correct). I would be nice to refactor that but I don't think that's the scope of this PR. Thanks for fixing this!
Yes, I agree. With this one and the next one, we will be close to it, but we will need more steps until having a good solution. |
Fix: #947 & #914