Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: save columns reference from sqllab save datasets flow #24248

Merged
merged 61 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
41a0312
fix save columns refernece
hughhhh May 30, 2023
844fbe9
ok
hughhhh May 30, 2023
02c01c2
allow schema to be null
hughhhh May 30, 2023
ef6cead
add support for legacy payload
hughhhh May 30, 2023
b257562
Merge branch 'master' into fix-save-ds-in-sqllab
hughhhh May 30, 2023
8769d6e
set default
hughhhh May 30, 2023
f0e3734
fix expression
hughhhh May 30, 2023
22223a7
ok
hughhhh May 30, 2023
84e307b
remove core change
hughhhh May 30, 2023
32920fd
update reference from API for column.column_name to match python column
hughhhh May 31, 2023
a107cee
ok
hughhhh May 31, 2023
fc741fa
ok
hughhhh May 31, 2023
49e10d2
ok
hughhhh May 31, 2023
29a224f
remove unneeded test
hughhhh Jun 1, 2023
510cd29
refactor on removing columns being sent on ds creation
hughhhh Jun 1, 2023
e69c1a0
revert test
hughhhh Jun 1, 2023
f95da35
lets see
hughhhh Jun 1, 2023
278bde6
lintint
hughhhh Jun 1, 2023
8fa92fd
update test
hughhhh Jun 1, 2023
51a0a38
ok
hughhhh Jun 1, 2023
4d62632
added test to verify name change
hughhhh Jun 1, 2023
d866479
console
hughhhh Jun 1, 2023
484c5f6
fix test
hughhhh Jun 2, 2023
dfc0b96
catch new sqllab endpoint
hughhhh Jun 2, 2023
5732bc4
Merge branch 'master' into fix-save-ds-in-sqllab
hughhhh Jun 5, 2023
7ab451a
oops
hughhhh Jun 5, 2023
bc60498
ok
hughhhh Jun 5, 2023
53d7cce
ok
hughhhh Jun 6, 2023
8506bb5
update another test
hughhhh Jun 6, 2023
f2b7040
ok
hughhhh Jun 6, 2023
93e2569
lit
hughhhh Jun 6, 2023
85f1e06
update inspector functions
hughhhh Jun 6, 2023
b7502f5
oops
hughhhh Jun 6, 2023
45d23ff
1 more
hughhhh Jun 6, 2023
7b17569
ok
hughhhh Jun 6, 2023
fe57567
fix integration config
hughhhh Jun 6, 2023
34b1f17
fix integration config
hughhhh Jun 6, 2023
c73464d
ok
hughhhh Jun 6, 2023
7317396
ok
hughhhh Jun 6, 2023
bb042b0
change name for column infor
hughhhh Jun 6, 2023
de8c023
preseto
hughhhh Jun 6, 2023
657c7cf
more
hughhhh Jun 6, 2023
4465785
1 more
hughhhh Jun 6, 2023
3f82bab
fixing more test
hughhhh Jun 6, 2023
d00e204
presto fix
hughhhh Jun 6, 2023
d4175a0
update spec
hughhhh Jun 7, 2023
78cd4e7
fix sync logic on the frontend
hughhhh Jun 7, 2023
bec68d0
fix merge conflict
hughhhh Jun 13, 2023
ed9728b
fix
hughhhh Jun 14, 2023
0d47277
linting
hughhhh Jun 14, 2023
8189c93
linting
hughhhh Jun 14, 2023
8895cff
oops
hughhhh Jun 14, 2023
4454cf3
1 more
hughhhh Jun 14, 2023
d96fbfd
fix more presto test
hughhhh Jun 14, 2023
bb8921a
ok
hughhhh Jun 14, 2023
cb6677d
ok fix
hughhhh Jun 14, 2023
6d3d955
omg
hughhhh Jun 14, 2023
a15cead
reference grandparent
hughhhh Jun 15, 2023
eeec51f
make schema backwards compatible
hughhhh Jun 15, 2023
743746e
Merge branch 'master' of https://github.com/preset-io/superset into f…
hughhhh Jun 20, 2023
2a1f09e
change refernce
hughhhh Jun 20, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -251,8 +251,7 @@ export const CtasEnum = {
};

export type QueryColumn = {
name: string;
column_name?: string;
column_name: string;
type: string | null;
is_dttm: boolean;
};
Expand Down Expand Up @@ -384,17 +383,17 @@ export const testQuery: Query = {
type: DatasourceType.Query,
columns: [
{
name: 'Column 1',
column_name: 'Column 1',
type: 'STRING',
is_dttm: false,
},
{
name: 'Column 3',
column_name: 'Column 3',
type: 'STRING',
is_dttm: false,
},
{
name: 'Column 2',
column_name: 'Column 2',
type: 'TIMESTAMP',
is_dttm: true,
},
Expand Down
3 changes: 2 additions & 1 deletion superset-frontend/src/SqlLab/actions/sqlLab.js
Original file line number Diff line number Diff line change
Expand Up @@ -1503,11 +1503,12 @@ export function createDatasourceFailed(err) {
export function createDatasource(vizOptions) {
return dispatch => {
dispatch(createDatasourceStarted());
const { dbId, schema, datasourceName, sql } = vizOptions;
const { dbId, schema, datasourceName, sql, columns } = vizOptions;
return SupersetClient.post({
endpoint: '/api/v1/dataset/',
headers: { 'Content-Type': 'application/json' },
body: JSON.stringify({
columns,
database: dbId,
schema,
sql,
Expand Down
6 changes: 3 additions & 3 deletions superset-frontend/src/SqlLab/components/ResultSet/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ const ResultSet = ({
...EXPLORE_CHART_DEFAULT,
datasource: `${results.query_id}__query`,
...{
all_columns: results.columns.map(column => column.name),
all_columns: results.columns.map(column => column.column_name),
},
});
const url = mountExploreUrl(null, {
Expand Down Expand Up @@ -491,7 +491,7 @@ const ResultSet = ({
}
if (data && data.length > 0) {
const expandedColumns = results.expanded_columns
? results.expanded_columns.map(col => col.name)
? results.expanded_columns.map(col => col.column_name)
: [];
return (
<>
Expand All @@ -500,7 +500,7 @@ const ResultSet = ({
{sql}
<FilterableTable
data={data}
orderedColumnKeys={results.columns.map(col => col.name)}
orderedColumnKeys={results.columns.map(col => col.column_name)}
height={rowsHeight}
filterText={searchText}
expandedColumns={expandedColumns}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export type ExploreQuery = QueryResponse & {

export interface ISimpleColumn {
column_name?: string | null;
name?: string | null;
type?: string | null;
is_dttm?: boolean | null;
}
Expand Down Expand Up @@ -199,6 +200,7 @@ export const SaveDatasetModal = ({
return;
}
setLoading(true);

const [, key] = await Promise.all([
updateDataset(
datasource?.dbId,
Expand All @@ -218,7 +220,9 @@ export const SaveDatasetModal = ({
...formDataWithDefaults,
datasource: `${datasetToOverwrite.datasetid}__table`,
...(defaultVizType === 'table' && {
all_columns: datasource?.columns?.map(column => column.column_name),
all_columns: datasource?.columns?.map(
column => column.name || column.column_name,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've asked this before, but I don't remember the answer. Why can't we standardize this? This is definitely going to cause bugs in the future, having two possible options for the attribute name.

Can't we normalize the schema early, if they have to be stored differently, instead of normalizing it late here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looking into this now @betodealmeida, i think it was just a difference between how sqllab consumes columns vs. explore

),
}),
}),
]);
Expand Down Expand Up @@ -305,7 +309,9 @@ export const SaveDatasetModal = ({
...formDataWithDefaults,
datasource: `${data.id}__table`,
...(defaultVizType === 'table' && {
all_columns: selectedColumns.map(column => column.column_name),
all_columns: selectedColumns.map(
column => column.name || column.column_name,
),
}),
}),
)
Expand Down
2 changes: 1 addition & 1 deletion superset/datasets/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ def post(self) -> Response:

try:
new_model = CreateDatasetCommand(item).run()
return self.response(201, id=new_model.id, result=item)
return self.response(201, id=new_model.id, result=item, data=new_model.data)
except DatasetInvalidError as ex:
return self.response_422(message=ex.normalized_messages())
except DatasetCreateFailedError as ex:
Expand Down
16 changes: 16 additions & 0 deletions superset/datasets/commands/create.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from sqlalchemy.exc import SQLAlchemyError

from superset.commands.base import BaseCommand, CreateMixin
from superset.connectors.sqla.models import SqlMetric, TableColumn
from superset.dao.exceptions import DAOCreateFailedError
from superset.datasets.commands.exceptions import (
DatabaseNotFoundValidationError,
Expand All @@ -44,8 +45,23 @@ def run(self) -> Model:
self.validate()
try:
# Creates SqlaTable (Dataset)
columns = self._properties.pop("columns", [])
dataset = DatasetDAO.create(self._properties, commit=False)
# Updates columns and metrics from the dataset
ds_columns = []
for column in columns:
column_name = column.get("column_name")
col = TableColumn(
column_name=column_name,
filterable=True,
groupby=True,
is_dttm=column.get("is_dttm", False),
type=column.get("type", False),
)
ds_columns.append(col)

dataset.columns = ds_columns
dataset.metrics = [SqlMetric(metric_name="count", expression="COUNT(*)")]
dataset.fetch_metadata(commit=False)
db.session.commit()
except (SQLAlchemyError, DAOCreateFailedError) as ex:
Expand Down
1 change: 1 addition & 0 deletions superset/datasets/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class DatasetPostSchema(Schema):
owners = fields.List(fields.Integer())
is_managed_externally = fields.Boolean(allow_none=True, dump_default=False)
external_url = fields.String(allow_none=True)
columns = fields.List(fields.Dict())


class DatasetPutSchema(Schema):
Expand Down
2 changes: 2 additions & 0 deletions superset/queries/dao.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ def update_saved_query_exec_info(query_id: int) -> None:
def save_metadata(query: Query, payload: Dict[str, Any]) -> None:
# pull relevant data from payload and store in extra_json
columns = payload.get("columns", {})
for col in columns:
col["column_name"] = col.pop("name")
Copy link
Member Author

@hughhhh hughhhh May 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from the query execution the dict comes back column[name], and decided to update the naming here before returning it to the client

db.session.add(query)
query.set_extra_json_key("columns", columns)

Expand Down
25 changes: 25 additions & 0 deletions tests/integration_tests/datasets/api_tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,31 @@ def test_create_dataset_item(self):
assert model.table_name == table_data["table_name"]
assert model.database_id == table_data["database"]

def test_create_dataset_item_with_columns(self):
"""
Dataset API: Test create dataset item with columns
"""
if backend() == "sqlite":
return

main_db = get_main_database()
self.login(username="admin")
table_data = {
"database": main_db.id,
"schema": None,
"table_name": "ab_permission",
"columns": [{"column_name": "foo", "type": "STRING", "is_dttm": False}],
}
uri = "api/v1/dataset/"
rv = self.post_assert_metric(uri, table_data, "post")
assert rv.status_code == 201
data = json.loads(rv.data.decode("utf-8"))
table_id = data.get("id")
model = db.session.query(SqlaTable).get(table_id)
assert model.table_name == table_data["table_name"]
assert model.database_id == table_data["database"]
assert len(model.columns) == 1

# Assert that columns were created
columns = (
db.session.query(TableColumn)
Expand Down