villebro commented on code in PR #18794:
URL: https://github.com/apache/superset/pull/18794#discussion_r873419204
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.tsx:
##########
@@ -44,6 +51,15 @@ const simpleAdhocFilter = new AdhocFilter({
clause: CLAUSES.WHERE,
});
+const businessTestAdhocFilterTest = new AdhocFilter({
Review Comment:
nit: this still references the old name
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx:
##########
@@ -276,7 +315,6 @@ const AdhocFilterEditPopoverSimpleTabContent:
React.FC<Props> = props => {
autoFocus: !subject,
placeholder: '',
};
-
Review Comment:
unnecessary change (I think the added blank line was ok)
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/useAdvancedDataTypes.ts:
##########
@@ -0,0 +1,105 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { useCallback, useState } from 'react';
+import { ensureIsArray, SupersetClient, t } from '@superset-ui/core';
+import { debounce } from 'lodash';
+import rison from 'rison';
+import { AdvancedDataTypesState, Props } from './index';
+
+const INITIAL_ADVANCED_DATA_TYPES_STATE: AdvancedDataTypesState = {
+ parsedAdvancedDataType: '',
+ advancedDataTypeOperatorList: [],
+ errorMessage: '',
+};
+
+const useAdvancedDataTypes = (validHandler: (isValid: boolean) => void) => {
+ const [advancedDataTypesState, setAdvancedDataTypesState] =
+ useState<AdvancedDataTypesState>(INITIAL_ADVANCED_DATA_TYPES_STATE);
+ const [subjectAdvancedDataType, setSubjectAdvancedDataType] = useState<
+ string | undefined
+ >();
+
+ const fetchAdvancedDataTypeValueCallback = useCallback(
+ (
+ comp: string | string[],
+ advancedDataTypesState: AdvancedDataTypesState,
+ subjectAdvancedDataType?: string,
+ ) => {
+ const values = ensureIsArray(comp);
+ console.log(values);
+ console.log(subjectAdvancedDataType);
Review Comment:
let's get rid of these
##########
superset/advanced_data_type/advanced_data_type.py:
##########
@@ -0,0 +1,46 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+AdvancedDataType type class
+"""
+
+from dataclasses import dataclass
+from typing import Any, Callable, List
+
+from sqlalchemy import Column
+from sqlalchemy.sql.expression import BinaryExpression
+
+from superset.advanced_data_type.advanced_data_type_request import (
+ AdvancedDataTypeRequest,
+)
+from superset.advanced_data_type.advanced_data_type_response import (
+ AdvancedDataTypeResponse,
+)
+from superset.utils.core import FilterOperator
+
+
+@dataclass
+class AdvancedDataType:
Review Comment:
nit: I'd almost rename this file to `types.py` if it will only include
classes and type definitions
##########
superset/connectors/sqla/models.py:
##########
@@ -1453,7 +1459,6 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
if get_column_name(flt_col) in removed_filters:
# Skip generating SQLA filter when the jinja template
handles it.
continue
-
Review Comment:
same here
##########
superset/advanced_data_type/advanced_data_type_request.py:
##########
@@ -0,0 +1,34 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+AdvancedDataType request class
+"""
+
+from typing import List, TypedDict, Union
+
+from superset.superset_typing import FilterValues
+
+
+class AdvancedDataTypeRequest(TypedDict):
Review Comment:
Since this module is already in the `advanced_data_type` package, the prefix
is implied. Hence I think `request.py` would suffice. However, to simplify, I'd
rather put this in `types.py`, as we probably don't need a separate module for
this.
##########
superset/advanced_data_type/advanced_data_type_response.py:
##########
@@ -0,0 +1,34 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""
+AdvancedDataType response class
+"""
+
+from typing import Any, List, Optional, TypedDict
+
+from superset.utils.core import FilterStringOperators
+
+
+class AdvancedDataTypeResponse(TypedDict, total=False):
Review Comment:
Same here (consolidate in `types.py`)
##########
superset/connectors/sqla/models.py:
##########
@@ -1432,7 +1439,6 @@ def get_sqla_query( # pylint:
disable=too-many-arguments,too-many-locals,too-ma
where_clause_and = []
having_clause_and = []
-
Review Comment:
I rather liked the blank line here
##########
superset-frontend/src/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx:
##########
@@ -233,6 +260,18 @@ const AdhocFilterEditPopoverSimpleTabContent:
React.FC<Props> = props => {
const [loadingComparatorSuggestions, setLoadingComparatorSuggestions] =
useState(false);
+ const {
+ advancedDataTypesState,
+ subjectAdvancedDataType,
+ fetchAdvancedDataTypeValueCallback,
+ fetchSubjectAdvancedDataType,
+ } = useAdvancedDataTypes(props.validHandler);
+ // TODO: This does not need to exist, just use the busninessTypeOperatorList
list
Review Comment:
same here
##########
tests/unit_tests/datasets/test_models.py:
##########
@@ -441,7 +441,7 @@ def test_create_physical_sqlatable(
}
-def test_create_virtual_sqlatable(
+def columns_default(
Review Comment:
I think this change is unnecessary
##########
tests/integration_tests/advanced_data_type/api_tests.py:
##########
@@ -0,0 +1,149 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+# isort:skip_file
+"""Unit tests for Superset"""
+import json
+import prison
+from sqlalchemy import null
+
+from superset.connectors.sqla.models import SqlaTable
+from superset.utils.core import get_example_default_schema
+
+from tests.integration_tests.base_tests import SupersetTestCase
+from tests.integration_tests.test_app import app
+from tests.integration_tests.utils.get_dashboards import get_dashboards_ids
+from unittest import mock
+from sqlalchemy import Column
+from typing import Any, List
+from superset.advanced_data_type.advanced_data_type import AdvancedDataType
+from superset.advanced_data_type.advanced_data_type_request import (
+ AdvancedDataTypeRequest,
+)
+from superset.advanced_data_type.advanced_data_type_response import (
+ AdvancedDataTypeResponse,
+)
+from superset.utils.core import FilterOperator, FilterStringOperators
+
+
+target_resp: AdvancedDataTypeResponse = {
+ "values": [],
+ "error_message": "",
+ "display_value": "",
+ "valid_filter_operators": [
+ FilterStringOperators.EQUALS,
+ FilterStringOperators.GREATER_THAN_OR_EQUAL,
+ FilterStringOperators.GREATER_THAN,
+ FilterStringOperators.IN,
+ FilterStringOperators.LESS_THAN,
+ FilterStringOperators.LESS_THAN_OR_EQUAL,
+ ],
+}
+
+
+def translation_func(req: AdvancedDataTypeRequest) -> AdvancedDataTypeResponse:
+ return target_resp
+
+
+def translate_filter_func(col: Column, op: FilterOperator, values: List[Any]):
+ pass
+
+
+test_type: AdvancedDataType = AdvancedDataType(
+ verbose_name="type",
+ valid_data_types=["int"],
+ translate_type=translation_func,
+ description="",
+ translate_filter=translate_filter_func,
+)
+
+CHART_DATA_URI = "api/v1/chart/advanced_data_type"
+CHARTS_FIXTURE_COUNT = 10
+
+
+class TestAdvancedDataTypeApi(SupersetTestCase):
Review Comment:
These would have been nice to write as functional tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
