Using inheritance scheme to organize db specific code #1294
Conversation
mistercrunch
force-pushed
the
db_engines
branch
from
e4fff21
to
f633b2e
Compare
|
@mistercrunch - could you please move this logic to that file too: |
| class BaseEngine(object): | ||
| engine = 'base' # str as defined in sqlalchemy.engine.engine | ||
| epoch_to_dttm = None | ||
|
|
There was a problem hiding this comment.
str as defined in sqlalchemy.engine.engine
I guess its sqlalchemy.engine.Engine.name
Describe the interface BaseEngine completely by adding
time_grains = () #Tuple of Grain. Time grains supported by the database.
Besides defining the used interface properly, it prevents the crash in model.Database.grain/grain_dict in the case of 'base' which is used if the SQL dialect is not listed here.
There was a problem hiding this comment.
good catch, I'm addressing this
| for db_type, grains in db_time_grains.items(): | ||
| if self.sqlalchemy_uri.startswith(db_type): | ||
| return grains | ||
| return self.db_engine.time_grains |
There was a problem hiding this comment.
Will crash with AttributeError in the case of the BaseEngine.
There was a problem hiding this comment.
I suggest renaming the db_engines.py/BaseEngine in someyhing like engine_specs or better dialect_specs since it just describes the SQL dialect. A SQLA-engine is a combination of dialect and connector/driver, e.g. mysql+oursql://... or mysql+mysqldb://.... .
There was a problem hiding this comment.
I would either expose db_engine as a property to the client and remove grains(), epoch_to_dttm(...) or make db_engine private and expose the functionality by grains(), ...
There was a problem hiding this comment.
By the way, can you add def query(...) to Models.Queryable to properly define that interface, too.
There was a problem hiding this comment.
I agree with the comments, though I had checked that
sqlalchemy.create_engine('mysql+mysqldb://').name == 'mysql' as opposed to mysql+mysqldb
I'm renaming to engine_spec / EngineSpec since we're planning on adding more there than dialect-specific things, like methods to access partitions lists and any other things that allow us to go around the "common denominator" limitations in SqlAlchemy.
About models.Queryable we need a deeper cleanup there and it's out of scope for this PR.
There was a problem hiding this comment.
I have checked
sqlalchemy.create_engine('mysql+mysqldb://').name== 'mysql',
too. That's why I assumed that you only specify dialects. A specific use case for a dialect+engine would be issue #236. This is mssql+pyodbc (and only pyodbc) specific.
There was a problem hiding this comment.
I think engine.name gets us what we need at least for now.
mistercrunch
force-pushed
the
db_engines
branch
from
f633b2e
to
611e570
Compare
|
addressed comments and fixed tests, merging |
Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.14.168 to 4.14.172. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash)
Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.14.168 to 4.14.172. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash)
Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.14.168 to 4.14.172. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash)
Bumps [@types/lodash](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/lodash) from 4.14.168 to 4.14.172. - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases) - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/lodash)
mistercrunch
added
🏷️ bot
First step in adding database-engine specific code. Next step is to use this structure to surface partition / latest partition metadata.