Cleanup rowid #48
Cleanup rowid #48
Conversation
pydanny
force-pushed
the
cleanup-rowid
branch
from
5aa095f to
5d70f8a
Compare
jph00
left a comment
jph00
left a comment
There was a problem hiding this comment.
Thanks Danny! My only question is about whether we really want all that code to auto-select 'id' for users. My guess is that we don't, but I might be missing something.
| ) | ||
|
|
||
| column_defs = [] | ||
| # All minidata tables get a primary key: https://docs.fastht.ml/explains/minidataapi.html#creating-tables |
There was a problem hiding this comment.
minutils does not support the spec - that's fastlite. We shouldn't add a pk here automatically IMO, unless I'm missing something.
There was a problem hiding this comment.
I'll work to get tests to pass without it.
| columns_to_drop = [ | ||
| column for column in existing_columns if column not in columns | ||
| ] | ||
| # If no primary key was specified and id was added automatically, we prevent |
There was a problem hiding this comment.
I guess we can remove this then?
| # Has the primary key changed? | ||
| current_pks = table.pks | ||
| desired_pk = None | ||
| if isinstance(pk, str): |
There was a problem hiding this comment.
You know, with a ternary op, these 4 lines would be just one line... ;)
There was a problem hiding this comment.
Oh n/m I see this was just a move of some lines Simon wrote.
| if transform and self[name].exists(): | ||
| table = cast(Table, self[name]) | ||
| should_transform = False | ||
| # Has the primary key changed? |
There was a problem hiding this comment.
I thought changing the pk already worked? IIRC I used that feature during development of solveit...
| defaults=defaults, | ||
| pk=pk, | ||
| ) | ||
| # There has been set a primary key that isn't ['id'].It is now safe to drop the ID column. |
| "Primary key columns for this table." | ||
| names = [column.name for column in self.columns if int(column.is_pk)] | ||
| if not names: | ||
| names = ["rowid"] |
There was a problem hiding this comment.
I don't think we want to force people to not use rowid do we?
| ignore, | ||
| ) | ||
|
|
||
| if isinstance(pk, Union[str, None]): |
There was a problem hiding this comment.
OK so this one could definitely be a ternary op!
Why does this exist?
insert_chunkwas still dependant on last_rowid in non-upsert scenariosWhat this does:
int:idto serve in this roleTests to be done: