Replace brush filters with MLv2 alternatives (#37293)

* Cleanup component

* Replace filters with MLv2 methods

* Cleanup unused functions

* Make filter work

* Make filter work

* Fix rebase issues

* Update ML methods usage

* pass params in the correct order

* pass params in the correct order

* Fix argumetns order

* Remove duplicated test and unskip correct one

* [Leaflet] Replace brush filter with MLv2 alternative (#37295)

* Update ML methods usage

* Fix argumetns order

* Unskip test

* Pass metadata

* Pass metadata

e2e/test/scenarios/visualizations-charts/line_chart.cy.spec.js

@@ -578,35 +578,7 @@ describe("scenarios > visualizations > line chart", () => {
     });
   });
 
-  it(
-    "should apply filters to the series selecting area range",
-    { tags: "@flaky" },
-    () => {
-      cy.viewport(1280, 800);
-
-      visitQuestionAdhoc({
-        dataset_query: testQuery,
-        display: "line",
-      });
-
-      cy.get(".Visualization")
-        .trigger("mousedown", 100, 200)
-        .trigger("mousemove", 230, 200)
-        .trigger("mouseup", 230, 200);
-
-      cy.wait("@dataset");
-
-      cy.findByTestId("filter-pill").should("contain.text", "Created At is");
-
-      const X_AXIS_VALUE = "June 2022";
-      cy.get(".CardVisualization").within(() => {
-        cy.get(".x-axis-label").should("have.text", "Created At");
-        cy.findByText(X_AXIS_VALUE);
-      });
-    },
-  );
-
-  it("should apply filters to the series selecting area range when axis is a number", () => {
+  it("should apply brush filters to the series selecting area range when axis is a number", () => {
     const testQuery = {
       type: "query",
       query: {

e2e/test/scenarios/visualizations-charts/maps.cy.spec.js

@@ -257,6 +257,7 @@ describe("scenarios > visualizations > maps", () => {
 
     cy.wait("@dataset");
 
+    cy.get(".CardVisualization").should("exist");
     // selecting area at the map provides different filter values, so the simplified assertion is used
     cy.findByTestId("filter-pill").should("have.length", 1);
   });

e2e/test/scenarios/visualizations-tabular/drillthroughs/chart_drill.cy.spec.js

@@ -29,7 +29,7 @@ describe("scenarios > visualizations > drillthroughs > chart drill", () => {
   it("should allow brush date filter", { tags: "@flaky" }, () => {
     cy.createQuestion(
       {
-        name: "Brush Date Filter",
+        name: "Brush Date Temporal Filter",
         query: {
           "source-table": ORDERS_ID,
           aggregation: [["count"]],
@@ -80,9 +80,6 @@ describe("scenarios > visualizations > drillthroughs > chart drill", () => {
 
   ["month", "month-of-year"].forEach(granularity => {
     it(`brush filter should work post-aggregation for ${granularity} granularity (metabase#18011)`, () => {
-      // TODO: Remove this line when the issue is fixed!
-      cy.skipOn(granularity === "month-of-year");
-
       cy.intercept("POST", "/api/dataset").as("dataset");
 
       const questionDetails = {

frontend/src/metabase-lib/queries/utils/actions.js

-// eslint-disable-next-line no-restricted-imports -- deprecated usage
-import moment from "moment-timezone";
-
-import { fieldRefForColumn } from "metabase-lib/queries/utils/dataset";
-import { FieldDimension } from "metabase-lib/Dimension";
-
-// STRUCTURED QUERY UTILITIES
-
-const fieldRefWithTemporalUnit = (mbqlClause, unit) => {
-  const dimension = FieldDimension.parseMBQLOrWarn(mbqlClause);
-  if (dimension) {
-    return dimension.withTemporalUnit(unit).mbql();
-  }
-  return mbqlClause;
-};
-
-function addOrUpdateFilter(query, newFilter) {
-  // replace existing filter, if it exists
-  for (const filter of query.filters()) {
-    const dimension = filter.dimension();
-    if (dimension && dimension.isSameBaseDimension(newFilter[1])) {
-      return filter.replace(newFilter);
-    }
-  }
-  // otherwise add a new filter
-  return query.filter(newFilter);
-}
-
-// min number of points when switching units
-const MIN_INTERVALS = 4;
-
-const UNITS = ["minute", "hour", "day", "week", "month", "quarter", "year"];
-const getNextUnit = unit => {
-  return UNITS[Math.max(0, UNITS.indexOf(unit) - 1)];
-};
-
-function addOrUpdateBreakout(query, newBreakout) {
-  // replace existing breakout, if it exists
-  for (const breakout of query.breakouts()) {
-    if (breakout.dimension().isSameBaseDimension(newBreakout)) {
-      return breakout.replace(newBreakout);
-    }
-  }
-  // otherwise add a new breakout
-  return query.breakout(newBreakout);
-}
-
-export function updateDateTimeFilter(query, column, start, end) {
-  const fieldRef = fieldRefForColumn(column);
-  start = moment(start);
-  end = moment(end);
-  if (column.unit) {
-    // start with the existing breakout unit
-    let unit = column.unit;
-
-    // clamp range to unit to ensure we select exactly what's represented by the dots/bars
-    start = start.add(1, unit).startOf(unit);
-    end = end.endOf(unit);
-
-    // find the largest unit with at least MIN_INTERVALS
-    while (
-      unit !== getNextUnit(unit) &&
-      end.diff(start, unit) < MIN_INTERVALS
-    ) {
-      unit = getNextUnit(unit);
-    }
-
-    // update the breakout
-    query = addOrUpdateBreakout(
-      query,
-      fieldRefWithTemporalUnit(fieldRef, unit),
-    );
-
-    // round to start of the original unit
-    start = start.startOf(column.unit);
-    end = end.startOf(column.unit);
-
-    if (start.isAfter(end)) {
-      return query;
-    }
-    if (start.isSame(end, column.unit)) {
-      // is the start and end are the same (in whatever the original unit was) then just do an "="
-      return addOrUpdateFilter(query, [
-        "=",
-        fieldRefWithTemporalUnit(fieldRef, column.unit),
-        start.format(),
-      ]);
-    } else {
-      // otherwise do a between
-      return addOrUpdateFilter(query, [
-        "between",
-        fieldRefWithTemporalUnit(fieldRef, column.unit),
-        start.format(),
-        end.format(),
-      ]);
-    }
-  } else {
-    return addOrUpdateFilter(query, [
-      "between",
-      fieldRef,
-      start.format(),
-      end.format(),
-    ]);
-  }
-}
-
-export function updateLatLonFilter(
-  query,
-  latitudeColumn,
-  longitudeColumn,
-  bounds,
-) {
-  return addOrUpdateFilter(query, [
-    "inside",
-    fieldRefForColumn(latitudeColumn),
-    fieldRefForColumn(longitudeColumn),
-    bounds.getNorth(),
-    bounds.getWest(),
-    bounds.getSouth(),
-    bounds.getEast(),
-  ]);
-}
-
-export function updateNumericFilter(query, column, start, end) {
-  const fieldRef = fieldRefForColumn(column);
-  return addOrUpdateFilter(query, ["between", fieldRef, start, end]);
-}

frontend/src/metabase/visualizations/components/LeafletMap.jsx

@@ -10,7 +10,7 @@ import "leaflet-draw";
 import _ from "underscore";
 
 import MetabaseSettings from "metabase/lib/settings";
-import { updateLatLonFilter } from "metabase-lib/queries/utils/actions";
+import * as Lib from "metabase-lib";
 import Question from "metabase-lib/Question";
 
 export default class LeafletMap extends Component {
@@ -147,6 +147,7 @@ export default class LeafletMap extends Component {
       ],
       settings,
       onChangeCardAndRun,
+      metadata,
     } = this.props;
 
     const latitudeColumn = _.findWhere(cols, {
@@ -156,16 +157,26 @@ export default class LeafletMap extends Component {
       name: settings["map.longitude_column"],
     });
 
-    const question = new Question(card);
+    const question = new Question(card, metadata);
     if (question.isStructured()) {
-      const nextCard = updateLatLonFilter(
-        question.legacyQuery({ useStructuredQuery: true }),
+      const query = question.query();
+      const stageIndex = -1;
+      const filterBounds = {
+        north: bounds.getNorth(),
+        south: bounds.getSouth(),
+        west: bounds.getWest(),
+        east: bounds.getEast(),
+      };
+      const updatedQuery = Lib.updateLatLonFilter(
+        query,
+        stageIndex,
         latitudeColumn,
         longitudeColumn,
-        bounds,
-      )
-        .question()
-        .card();
+        filterBounds,
+      );
+      const updatedQuestion = question.setQuery(updatedQuery);
+      const nextCard = updatedQuestion.card();
+
       onChangeCardAndRun({ nextCard });
     }
 

frontend/src/metabase/visualizations/components/LineAreaBarChart.jsx

@@ -89,19 +89,6 @@ export default class LineAreaBarChart extends Component {
     }
   }
 
-  static propTypes = {
-    card: PropTypes.object.isRequired,
-    series: PropTypes.array.isRequired,
-    settings: PropTypes.object.isRequired,
-    actionButtons: PropTypes.node,
-    showTitle: PropTypes.bool,
-    isDashboard: PropTypes.bool,
-    headerIcon: PropTypes.object,
-    width: PropTypes.number,
-  };
-
-  static defaultProps = {};
-
   getHoverClasses() {
     const { hovered } = this.props;
     if (hovered && hovered.index != null) {
@@ -321,6 +308,17 @@ export default class LineAreaBarChart extends Component {
   }
 }
 
+LineAreaBarChart.propTypes = {
+  card: PropTypes.object.isRequired,
+  series: PropTypes.array.isRequired,
+  settings: PropTypes.object.isRequired,
+  actionButtons: PropTypes.node,
+  showTitle: PropTypes.bool,
+  isDashboard: PropTypes.bool,
+  headerIcon: PropTypes.object,
+  width: PropTypes.number,
+};
+
 function transformSingleSeries(s, series, seriesIndex) {
   const { card, data } = s;
 

frontend/src/metabase/visualizations/lib/LineAreaBarRenderer.js

@@ -7,10 +7,7 @@ import { t } from "ttag";
 import { lighten } from "metabase/lib/colors";
 
 import { keyForSingleSeries } from "metabase/visualizations/lib/settings/series";
-import {
-  updateDateTimeFilter,
-  updateNumericFilter,
-} from "metabase-lib/queries/utils/actions";
+import * as Lib from "metabase-lib";
 import { isStructured } from "metabase-lib/queries/utils/card";
 import Question from "metabase-lib/Question";
 
@@ -351,7 +348,7 @@ function getYAxisProps(props, yExtents, datas) {
 
 /// make the `onBrushChange()` and `onBrushEnd()` functions we'll use later, as well as an `isBrushing()` function to check
 /// current status.
-function makeBrushChangeFunctions({ series, onChangeCardAndRun }) {
+function makeBrushChangeFunctions({ series, onChangeCardAndRun, metadata }) {
   let _isBrushing = false;
 
   const isBrushing = () => _isBrushing;
@@ -362,25 +359,44 @@ function makeBrushChangeFunctions({ series, onChangeCardAndRun }) {
 
   function onBrushEnd(range) {
     _isBrushing = false;
+
     if (range) {
       const column = series[0].data.cols[0];
       const card = series[0].card;
-      const query = new Question(card).legacyQuery({
-        useStructuredQuery: true,
-      });
+      const question = new Question(card, metadata);
+      const query = question.query();
+      const stageIndex = -1;
+
       const [start, end] = range;
+
       if (isDimensionTimeseries(series)) {
+        const nextQuery = Lib.updateTemporalFilter(
+          query,
+          stageIndex,
+          column,
+          new Date(start).toISOString(),
+          new Date(end).toISOString(),
+        );
+        const updatedQuestion = question.setQuery(nextQuery);
+        const nextCard = updatedQuestion.card();
+
         onChangeCardAndRun({
-          nextCard: updateDateTimeFilter(query, column, start, end)
-            .question()
-            .card(),
+          nextCard,
           previousCard: card,
         });
       } else {
+        const nextQuery = Lib.updateNumericFilter(
+          query,
+          stageIndex,
+          column,
+          start,
+          end,
+        );
+        const updatedQuestion = question.setQuery(nextQuery);
+        const nextCard = updatedQuestion.card();
+
         onChangeCardAndRun({
-          nextCard: updateNumericFilter(query, column, start, end)
-            .question()
-            .card(),
+          nextCard,
           previousCard: card,
         });
       }

src/metabase/lib/js.cljs

@@ -1234,15 +1234,15 @@
 
 (defn ^:export update-numeric-filter
   "Add or update a filter against `numeric-column`."
-  [a-query numeric-column stage-number start end]
+  [a-query stage-number numeric-column start end]
   (let [numeric-column (legacy-column->metadata a-query stage-number numeric-column)]
-    (lib.core/update-numeric-filter a-query numeric-column stage-number start end)))
+    (lib.core/update-numeric-filter a-query stage-number numeric-column start end)))
 
 (defn ^:export update-temporal-filter
   "Add or update a filter against `temporal-column`. Modify the temporal unit for any breakouts."
-  [a-query temporal-column stage-number start end]
+  [a-query stage-number temporal-column start end]
   (let [temporal-column (legacy-column->metadata a-query stage-number temporal-column)]
-    (lib.core/update-temporal-filter a-query temporal-column stage-number start end)))
+    (lib.core/update-temporal-filter a-query stage-number temporal-column start end)))
 
 (defn ^:export valid-filter-for?
   "Given two CLJS `:metadata/columns` returns true if `src-column` is a valid source to use for filtering `dst-column`."