fix(explore): Explore page boolean filter is broken for Presto DB #14952

m-ajay · 2021-06-02T18:08:46Z

SUMMARY

Explore page boolean filter was not working for presto db even after the PR #14567. #10098

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

IS_TRUE filter

IS_TRUE false

TESTING INSTRUCTIONS

Manual and CI

ADDITIONAL INFORMATION

Has associated issue:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

…y value

graceguo-supercat · 2021-06-03T07:09:15Z

@m-ajay Thanks for the fix. I tested this PR in dev environment. it works as expected. You can add more description and it will be ready for code review.

codecov · 2021-06-08T02:58:05Z

Codecov Report

Merging #14952 (e221f7f) into master (21aa3da) will decrease coverage by 0.15%.
The diff coverage is 73.10%.

❗ Current head e221f7f differs from pull request most recent head f8638a9. Consider uploading reports for the commit f8638a9 to get more accurate results

@@            Coverage Diff             @@
##           master   #14952      +/-   ##
==========================================
- Coverage   77.51%   77.36%   -0.16%     
==========================================
  Files         966      967       +1     
  Lines       49615    49740     +125     
  Branches     6311     6351      +40     
==========================================
+ Hits        38458    38479      +21     
- Misses      10956    11059     +103     
- Partials      201      202       +1

Flag	Coverage Δ
javascript	`72.44% <73.05%> (-0.34%)`	⬇️
mysql	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset-frontend/src/common/components/index.tsx	`100.00% <ø> (ø)`
superset-frontend/src/components/Button/index.tsx	`100.00% <ø> (ø)`
...c/components/ErrorMessage/DatabaseErrorMessage.tsx	`94.73% <ø> (ø)`
...onents/ErrorMessage/ErrorMessageWithStackTrace.tsx	`77.77% <0.00%> (ø)`
.../components/ErrorMessage/ParameterErrorMessage.tsx	`96.87% <ø> (ø)`
...rset-frontend/src/components/ErrorMessage/types.ts	`100.00% <ø> (ø)`
...et-frontend/src/components/Select/NativeSelect.tsx	`100.00% <ø> (ø)`
...tersConfigModal/FiltersConfigForm/DefaultValue.tsx	`25.00% <0.00%> (-1.32%)`	⬇️
...-frontend/src/explore/components/ControlHeader.jsx	`85.71% <ø> (ø)`
...trols/DateFilterControl/components/CustomFrame.tsx	`92.15% <ø> (ø)`
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21aa3da...f8638a9. Read the comment docs.

etr2460

some comments, nice work so far!

etr2460 · 2021-06-08T15:02:24Z

superset-frontend/src/explore/constants.ts

+  operation: string;
+}
+
+export const OPERATOR_MAPPING: {


Not sure about this naming, perhaps OPERATOR_ENUM_TO_OPERATOR_TYPE?

etr2460 · 2021-06-08T15:03:20Z

superset-frontend/src/explore/constants.ts

-  'IS TRUE': 'IS TRUE',
-  'IS FALSE': 'IS FALSE',
+export enum Operators {
+  EQUALS = 1,


if we're using this as an id, then for ease of debugging, i think i'd prefer using

EQUALS = 'EQUALS', NOT_EQUALS = 'NOT_EQUALS', etc.

etr2460 · 2021-06-08T15:05:37Z

...ntrol/AdhocFilterEditPopoverSimpleTabContent/AdhocFilterEditPopoverSimpleTabContent.test.tsx

+  sumValueAdhocMetric,
+];
+
+function setup(overrides?: object) {


Prefer Record<string, any> to object

etr2460 · 2021-06-08T15:13:20Z

...c/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx

+  };
+  const onSubjectChange = (id: string) => {
+    const option = props.options.find(
+      // @ts-ignore


Why is the ts-ignore needed here? Can we type option as ColumnType to get around it?

etr2460 · 2021-06-08T15:14:59Z

...c/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx

+    isOperatorRelevant,
+    onComparatorChange,
+  } = useSimpleTabFilterProps(props);
+  const [suggestions, setSuggestions] = useState<JsonObject>([]);


is JsonObject the right thing to use here since you initialize to an empty array? Perhaps Record<string, any>[] is correct?

etr2460 · 2021-06-08T15:16:18Z

...c/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx

+  const [abortActiveRequest, setAbortActiveRequest] = useState<
+    (() => void) | null
+  >(null);


maybe out of scope, but initializing this to NO_OP (or maybe noOp, I forget how it's named in our utils file, it's basically an empty function) would prevent the need to manually set the type and to check if abortActiveRequest exists before calling it everytime. Seems cleaner

etr2460 · 2021-06-08T15:17:10Z

...c/explore/components/controls/FilterControl/AdhocFilterEditPopoverSimpleTabContent/index.tsx

+          name="filter-value"
+          ref={ref =>
+            focusComparator(
+              ref as HTMLInputElement | null,


ideally we don't need to force the typing here, but it might be tricky

etr2460 · 2021-06-08T15:17:25Z

superset-frontend/src/explore/components/controls/FilterControl/adhocFilterType.js

 import { EXPRESSION_TYPES, CLAUSES } from './AdhocFilter';

 export default PropTypes.oneOfType([
  PropTypes.shape({
    expressionType: PropTypes.oneOf([EXPRESSION_TYPES.SIMPLE]).isRequired,
    clause: PropTypes.oneOf([CLAUSES.HAVING, CLAUSES.WHERE]).isRequired,
    subject: PropTypes.string.isRequired,
-    operator: PropTypes.oneOf(Object.keys(OPERATORS)).isRequired,
+    // TODO: Fix the type
+    // operator: PropTypes.oneOf(Object.keys(OPERATORS)).isRequired,


looks like we need to fix something here?

etr2460 · 2021-06-08T15:17:43Z

superset-frontend/src/explore/components/controls/MetricControl/FilterDefinitionOption.jsx

@@ -51,5 +51,6 @@ export default function FilterDefinitionOption({ option }) {
      />
    );
  }
+  return null;


etr2460

no more code review comments, but it looks like there are some frontend lint errors still? I believe running npm run lint-fix locally will help you find/autofix them

etr2460

woohoo!

…ache#14952) * Front end update - modify OPERATORS, to have SQL operation and display value * Updated tests * More tests * Remove OPERATOR imports * Fix break tests * PR comments * fix issue with comparator loading * rename a variable * Linting

m-ajay added 2 commits June 2, 2021 13:37

Front end update - modify OPERATORS, to have SQL operation and displa…

4c4d5f2

…y value

Conflict resolution

8462084

pull-request-size bot added the size/XXL label Jun 2, 2021

m-ajay changed the title ~~[WIP] (fix)explore: Explore page boolean filter is broken for Preset DB~~ [WIP] (fix)explore: Explore page boolean filter is broken for Presto DB Jun 2, 2021

m-ajay added 2 commits June 7, 2021 14:14

Updated tests

08ee4c4

More tests

438d6bf

m-ajay changed the title ~~[WIP] (fix)explore: Explore page boolean filter is broken for Presto DB~~ (fix)explore: Explore page boolean filter is broken for Presto DB Jun 7, 2021

m-ajay marked this pull request as ready for review June 7, 2021 20:00

m-ajay changed the title ~~(fix)explore: Explore page boolean filter is broken for Presto DB~~ fix(explore): Explore page boolean filter is broken for Presto DB Jun 7, 2021

m-ajay added 2 commits June 7, 2021 16:47

Remove OPERATOR imports

6b2e2ff

Fix break tests

5881b2d

graceguo-supercat requested a review from ktmud June 7, 2021 23:25

Merge branch 'master' into fix/boolean-filter-not-working-preset-db

153dc84

etr2460 reviewed Jun 8, 2021

View reviewed changes

PR comments

987088f

m-ajay requested a review from etr2460 June 9, 2021 15:44

m-ajay changed the title ~~fix(explore): Explore page boolean filter is broken for Presto DB~~ [WIP] fix(explore): Explore page boolean filter is broken for Presto DB Jun 9, 2021

m-ajay added 2 commits June 9, 2021 18:13

fix issue with comparator loading

a37057b

rename a variable

8fa4259

m-ajay changed the title ~~[WIP] fix(explore): Explore page boolean filter is broken for Presto DB~~ fix(explore): Explore page boolean filter is broken for Presto DB Jun 9, 2021

etr2460 reviewed Jun 9, 2021

View reviewed changes

Linting

f8638a9

etr2460 approved these changes Jun 10, 2021

View reviewed changes

etr2460 merged commit f8b270d into apache:master Jun 10, 2021

mistercrunch added the 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels label Mar 12, 2024

mistercrunch added the 🚢 1.3.0 label Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(explore): Explore page boolean filter is broken for Presto DB #14952

fix(explore): Explore page boolean filter is broken for Presto DB #14952

m-ajay commented Jun 2, 2021 •

edited

graceguo-supercat commented Jun 3, 2021

codecov bot commented Jun 8, 2021 •

edited

etr2460 left a comment

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 Jun 8, 2021

etr2460 left a comment

etr2460 left a comment

fix(explore): Explore page boolean filter is broken for Presto DB #14952

fix(explore): Explore page boolean filter is broken for Presto DB #14952

Conversation

m-ajay commented Jun 2, 2021 • edited

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

graceguo-supercat commented Jun 3, 2021

codecov bot commented Jun 8, 2021 • edited

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

m-ajay commented Jun 2, 2021 •

edited

codecov bot commented Jun 8, 2021 •

edited