ktmud
commented
2 years ago I like this! Changing superset/{datasets,dashboard...}/{api,command,dao} to superset/{api,commands,daos}/{dataset,dashboard,...} would at least reduce the amount of top-level folders and make it easier to navigate for people who are just working on the APIs. And more often than not, you'd probably work on APIs utilizing multiple models rather than working on the models AND the API of a single entity type at the same time.
I also like that the core SQL engine gets its own folder. It's long overdue.
One thing I'm not sure about is can we make it faster to find all models that correspond to a db table? I assume they will all be moved to dao/..., but is the nesting under dao/{entity_type}/models.py too deep still?
Another thing that has been bothering me is---there seems to be too many boilerplates and parameters passing around between the API, the commands and the DAO. Also, what is the boundary between a command and a DAO method anyway? Currently it seems a very thick validation layer can be added to either the DAO or a command.
So I wonder if we can further simplify things down to something like this:
superset
├── api
│ ├── legacy
│ └── v1
├── views
│ └── legacy
│ ├── core.py
│ └── ...
├── tasks
│ └── ...
├── commands
│ ├── datasets
│ ├── dashboards
│ │ ├── create_dashboard.py
│ │ ├── get_dashboard_charts.py
│ │ └── ...
│ ├── reports
│ └── ...
├── engine
│ ├── specs
│ ├── query
│ │ ├── builder
│ │ ├── executor
│ │ └── results
├── models
│ ├── base.py
│ ├── chart.py
│ ├── dashboard.py
│ ├── dataset.py
│ ├── query.py
│ ├── report.py
│ └── ...
└── utilsA couple of things to note:
- Input validation that does not require pulling data from db (e.g.) would stay with the API---ideally done by the API framework directly via custom validators (using Marshmallow or any other validation library).
commandswill include any data operations (query or mutation) that require more than one model. Classes inmodelsshould try their best to not import any other models unless the dependency is strictly one directional (i.e., it's a 1-many relationship instead of many-to-many, e.g., dataset -> columns).- Validations before data writes should happen privately within the data writer. There should be no need to export the validation methods---so we can avoid passing parameters around for too many layers.
john-bodley
michael-s-molina
craig-rueda
rusackas
[SIP-92] Proposal for restructuring the Python code base
Motivation
Superset has evolved somewhat organically over time which is reflected—somewhat apparently—in the how the Python code—which resides solely in the top level
supersetfolder—is organized. Initially Superset used a Model View Controlled (MVC) pattern combined with the notion of database connectors whereas now we've adopted the Data Access Object (DAO) pattern (SIP-35), which when coupled with commands and the API, helps to decouple the business layer from the persistence layer.Due to partial refactors and years of creep the code organization is fragmented. This has negatively impacted both the code quality and developer experience.
Proposed Change
The TL;DR is the Superset application is primarily composed on a few major functional components:
The proposed change would be to refactor the code into more functional rather than business top level folders, which has become somewhat bloated and results in a fragmented core—something which has plagued Superset since its inception with its feature driven mindset at the cost of architectural integrity.
Below is the before/after enumeration of the current (as of 10/28/2022) top-level folders and files, where "N/A" denotes that the folder/files will no longer exist in its current form. The additional sub-sections outline more specifics.
APIs, Commands, DAOs, and Models
APIs historically were mostly defined in an ad-hoc manner, i.e., in a non-RESTful way, as views which mostly reside in within the
superset/views/folder. These "legacy" APIs now coexist alongside RESTful APIs which leverage the DAO model which reside in the component specific folder, i.e.,superset/datasets/. Furthermore commands are either defined within thesuperset/commands/folder or component specific folder, i.e.,superset/datasets/commands.As a developer it isn't overly apparent where an API endpoint resides. The proposed solution is move to a directory structure which more clearly illustrates that the APIs, DAOs, and commands are decoupled (as illustrated below for the datasets components). The API—which leverages blueprints—is comprised both of the v1 RESTful API (current) and the legacy API. This demarcation also helps developers identify which API endpoints need to be migrated to v1.
Note currently views are a combination of legacy API endpoints and non-API endpoints. The concept of views will remain but should only contain non-API endpoints, i.e., rendering HTML templates.
SQL Engine
Though not as well flushed out as APIs, commands, and DAOs, the actual SQL engine—responsible for preparing, executing, fetching result sets—would be colocated within the broad
superset/engine/subfolder. This would comprise of the engine specifications, templating, SQL parsing, query objects, etc.New or Changed Public Interfaces
N/A.
New dependencies
N/A.
Migration Plan and Compatibility
The code restructure can be piecemeal. The general steps are:
Rejected Alternatives
Regarding the engine directory structure I'm unsure whether the current proposal makes the most sense. An alternative could be based more on the flow/path of a query from pre-processing (including construction and SQL parsing), to execution, then fetching, and finally post-processing of the result set.
I presume that calling out the engine as a first class entity rather than treating it as a command likely makes sense. I think this is open for debate/discussion.