chore: removing Druid from front- and back- end #20338

AAfghahi · 2022-06-09T19:19:53Z

SUMMARY

This is the first pass at the Druid specific conditionals.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
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

codecov · 2022-06-09T19:29:48Z

Codecov Report

Merging #20338 (4be9f7e) into master (4bfa622) will increase coverage by 0.02%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##           master   #20338      +/-   ##
==========================================
+ Coverage   66.84%   66.86%   +0.02%     
==========================================
  Files        1754     1753       -1     
  Lines       65731    65664      -67     
  Branches     6950     6921      -29     
==========================================
- Hits        43940    43909      -31     
+ Misses      20028    19997      -31     
+ Partials     1763     1758       -5

Flag	Coverage Δ
hive	`53.79% <ø> (+<0.01%)`	⬆️
javascript	`51.85% <81.25%> (+0.01%)`	⬆️
mysql	`82.42% <ø> (+0.01%)`	⬆️
postgres	`82.50% <ø> (+0.01%)`	⬆️
presto	`53.65% <ø> (+<0.01%)`	⬆️
python	`82.93% <ø> (+0.01%)`	⬆️
sqlite	`82.28% <ø> (+0.01%)`	⬆️
unit	`50.78% <ø> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...ckages/superset-ui-chart-controls/src/constants.ts	`100.00% <ø> (ø)`
...perset-ui-chart-controls/src/sections/sections.tsx	`100.00% <ø> (ø)`
...et-ui-chart-controls/src/shared-controls/index.tsx	`51.26% <ø> (-0.41%)`	⬇️
...ges/superset-ui-core/src/query/buildQueryObject.ts	`100.00% <ø> (ø)`
...ges/superset-ui-core/src/query/extractTimegrain.ts	`100.00% <ø> (ø)`
...kages/superset-ui-core/src/query/getMetricLabel.ts	`100.00% <ø> (ø)`
...kages/superset-ui-core/src/query/processFilters.ts	`91.66% <ø> (-8.34%)`	⬇️
...ages/superset-ui-core/src/query/types/Dashboard.ts	`100.00% <ø> (ø)`
...superset-ui-core/src/query/types/PostProcessing.ts	`100.00% <ø> (ø)`
...packages/superset-ui-core/src/query/types/Query.ts	`100.00% <ø> (ø)`
... and 27 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 4bfa622...4be9f7e. Read the comment docs.

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

superset/charts/schemas.py

superset/common/query_object.py

superset/utils/core.py

superset/viz.py

eschutho

looking good so far.. thanks @AAfghahi!

betodealmeida · 2022-06-09T21:43:28Z

superset-frontend/packages/superset-ui-core/test/query/processFilters.test.ts

-          {
-            expressionType: 'SIMPLE',
-            clause: 'HAVING',
-            subject: 'sweetness',
-            operator: '>',
-            comparator: '0',
-          },
-          {
-            expressionType: 'SIMPLE',
-            clause: 'HAVING',
-            subject: 'sweetness',
-            operator: '<=',
-            comparator: '50',
-          },


Are these Druid specific?

I think so, because it maps to a field called DRUID_ONLY_OPERATORS however they seem like relatively normal operators to have.

Ah, right, I think expressionType=SIMPLE is Druid-only.

betodealmeida · 2022-06-09T21:45:18Z

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

    return !(
-      (props.datasource.type === 'druid' &&
-        TABLE_ONLY_OPERATORS.indexOf(operator) >= 0) ||
-      (props.datasource.type === 'table' &&
-        DRUID_ONLY_OPERATORS.indexOf(operator) >= 0) ||
-      (props.adhocFilter.clause === CLAUSES.HAVING &&
-        HAVING_OPERATORS.indexOf(operator) === -1)
+      props.adhocFilter.clause === CLAUSES.HAVING &&
+      HAVING_OPERATORS.indexOf(operator) === -1


Might be cleaner to write as:

return ( props.adhocFilter.clause !== CLAUSES.HAVING || HAVING_OPERATORS.indexOf(operator) !== -1 );

I like that thank you!

You can always replace !(a && b) with !a || !b, and in this case I think it makes it easier to follow the logic.

betodealmeida · 2022-06-09T21:48:07Z

superset/utils/core.py

-            rejected.append(
-                {
-                    "reason": ExtraFiltersReasonType.NOT_DRUID_DATASOURCE,
-                    "column": ExtraFiltersTimeColumnType.GRANULARITY,
-                }
-            )


We should keep this, no?

just the else?

Right, the else.

Or maybe not, ExtraFiltersReasonType.NOT_DRUID_DATASOURCE suggests that this whole part of the code is Druid only.

Thinking about it more, I feel as though these two are pretty druid specific.

betodealmeida · 2022-06-09T21:48:59Z

superset/utils/core.py

+        # This seems like it is to do with the druid time series, which I think we
+        # are still keeping mentions of.


Can you clarify this?

Yes! This was a misunderstanding from earlier today. I thought that we were keeping druid_time_origin and we are not.

AAfghahi · 2022-06-10T16:49:50Z

superset-frontend/packages/superset-ui-core/src/query/extractTimegrain.ts

@@ -23,7 +23,7 @@ import { TimeGranularity } from '../time-format';

 export default function extractTimegrain(
  formData: QueryFormData,
-): TimeGranularity | undefined {
+): TimeGranularity | undefined | string {


I am not excited about changing this to include strings, even though it seems to be pretty 50/50 in the codebase. Is it worth to go through and refactor the string portions @eschutho

superset-frontend/src/explore/controlUtils/standardizedFormData.test.tsx

AAfghahi · 2022-07-06T16:26:30Z

/testenv up

github-actions · 2022-07-06T16:46:51Z

@AAfghahi Ephemeral environment spinning up at http://35.89.28.211:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset-frontend/src/explore/controlUtils/getSectionsToRender.ts

eschutho

code looks good. Thanks @AAfghahi! Let's see if @jinghua-qa is available to QA before merging.

zhaoyongjie · 2022-07-07T09:05:35Z

@AAfghahi the superset-ui/core should make 100% code coverage. the #58 in processFilters.ts needs some test coverage. Oneline checks code coverage locally

$ npx jest --coverage --collectCoverageFrom='["packages/**/src/**/*.{js,ts}", "!packages/superset-ui-demo/**/*"]' packages

zhaoyongjie

Thanks for the hard work. This PR makes a lot of work easier.

superset-frontend/packages/superset-ui-core/src/query/types/QueryFormData.ts

zhaoyongjie · 2022-07-07T09:15:53Z

superset-frontend/packages/superset-ui-core/test/query/types/Metric.test.ts

-  expect(isSavedMetric(adhocMetricSQL)).toEqual(false);
-  expect(isSavedMetric(null)).toEqual(false);
-  expect(isSavedMetric(undefined)).toEqual(false);
+  expect(isSavedMetric(adhocMetricSQL as QueryFormMetric)).toEqual(false);
+  expect(isSavedMetric(null as unknown as QueryFormMetric)).toEqual(false);
+  expect(isSavedMetric(undefined as unknown as QueryFormMetric)).toEqual(false);


the type of argument(metric) is any, so no needs as QueryFormMetric

export function isSavedMetric(metric: any): metric is SavedMetric { return typeof metric === 'string'; }

zhaoyongjie · 2022-07-07T09:16:43Z

superset-frontend/packages/superset-ui-core/src/query/types/index.ts

@@ -19,10 +19,8 @@
 export * from './Datasource';
 export * from './Column';
 export * from './Filter';
-export * from './Metric';


should keep this line.

superset-frontend/packages/superset-ui-core/src/query/types/index.ts

…-nosql-from-the-codebase

AAfghahi · 2022-07-07T16:15:53Z

/testenv up

github-actions · 2022-07-07T16:20:08Z

@AAfghahi Ephemeral environment spinning up at http://35.89.191.195:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

andrey-zayats · 2022-07-08T15:55:29Z

QA tested and verified. LGTM

github-actions · 2022-07-08T15:57:26Z

Ephemeral environment shutdown and build artifacts deleted.

* first pass at removing native Druid nosql * removing having_druid * addressing comments, linting * fixed all tests * addressing comments * redirected to ui-core TimeGranularity type * query form metric linting * fixed broken chart type * implementing feedback

superset-github-bot bot added the preset-io label Jun 9, 2022

pull-request-size bot added the size/L label Jun 9, 2022

AAfghahi force-pushed the arash.afghahi/sc-46372/remove-native-druid-nosql-from-the-codebase branch 2 times, most recently from 4ae9553 to c37253c Compare June 9, 2022 19:50