-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Migrate notebook aggregations to MLv2 — Metrics (2) #31528
Migrate notebook aggregations to MLv2 — Metrics (2) #31528
Conversation
a26c5ae
to
3146d22
Compare
Current Cypress Test Results Summary✅ 1699 Passing - ❌ 13 Failing - Run may still be in progress, this comment will be updated as current testing workflow or job completes... (Last updated on 06/28/2023 01:05:59pm UTC) Run DetailsRunning Workflow E2E Tests on Github Actions Commit: 600a55344f5be83c1efc962ad3122fa9274c5c84 Started: 06/28/2023 12:28:04pm UTC ❌ Failures📄 e2e/test/scenarios/question/summarization.cy.spec.js • 3 FailuresTop 1 Common Error Messages
Test Case Results
📄 e2e/test/scenarios/question/notebook.cy.spec.js • 3 FailuresTop 1 Common Error Messages
Test Case Results
📄 e2e/test/scenarios/question/reproductions/6239-sort-using-cust-exp.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/question/reproductions/18207-string-min-max.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/question/reproductions/17512.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/models/reproductions/29951-model-editor-results-metadata.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/question/reproductions/15714-cc-postgres-percentile-accepts-two-params.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/filters/filter.cy.spec.js • 1 FailureTest Case Results
📄 e2e/test/scenarios/custom-column/custom-column.cy.spec.js • 1 FailureTest Case Results
|
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
scenarios > models listing should allow adding models to dashboards
Retry 1 • Initial Attempt |
1.29% (6)6 / 465 runsfailed over last 7 days |
9.03% (42)42 / 465 runsflaked over last 7 days |
📄 e2e/test/scenarios/actions/model-actions.cy.spec.js • 2 Flakes
Top 1 Common Error Messages
|
2 Test Cases Affected |
Test Case Results
Test Case | Last 7 days Failures | Last 7 days Flakes |
---|---|---|
Write actions on model detail page (postgres) should allow public sharing of actions and execution of public actions
Retry 2 • Retry 1 • Initial Attempt |
1.38% (6)6 / 436 runsfailed over last 7 days |
83.49% (364)364 / 436 runsflaked over last 7 days |
Write actions on model detail page (postgres) hidden fields should allow implicit action execution from the model details page
Retry 2 • Retry 1 • Initial Attempt |
1.07% (4)4 / 373 runsfailed over last 7 days |
82.57% (308)308 / 373 runsflaked over last 7 days |
📄 e2e/test/scenarios/dashboard/dashboard-card-resizing.cy.spec.js • 1 Flake
Test Case Results
(truncated)...
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## mlv2-fe/aggregations-notebook #31528 +/- ##
================================================================
Coverage ? 74.21%
================================================================
Files ? 2907
Lines ? 103416
Branches ? 12842
================================================================
Hits ? 76746
Misses ? 20858
Partials ? 5812
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fbf65c5
to
f479386
Compare
3146d22
to
ac5a7ea
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a heads up that I reviewed this PR but disagree with the approach to use MLv1 here. Discussing it here https://metaboat.slack.com/archives/C04CYTEL9N2/p1686749310644099
f479386
to
92802cf
Compare
f35a48a
to
92f20af
Compare
e60cc6f
to
1dcfd0f
Compare
92f20af
to
71da7a3
Compare
1dcfd0f
to
f959cdc
Compare
71da7a3
to
30836ac
Compare
f959cdc
to
7e9a03c
Compare
30836ac
to
b7e06ce
Compare
@ranquild I've updated the PR to use MLv2-based metrics, please take another look |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome!
* Add basic legacy metrics support * Don't show archived metrics * Highlight selected metric * Extract `AggregationPopover` in notebook step * Fix `onClose` prop * Fix icon * Add missing metric types * Use MLv2 to manage metrics * Remove legacy query logic * Add `Aggregatable` type
* Add basic legacy metrics support * Don't show archived metrics * Highlight selected metric * Extract `AggregationPopover` in notebook step * Fix `onClose` prop * Fix icon * Add missing metric types * Use MLv2 to manage metrics * Remove legacy query logic * Add `Aggregatable` type
* Migrate notebook aggregations to MLv2 — Operators (1) (#31527) * Add aggregation types * Add aggregation TypeScript wrappers * Move `AggregateStep` to its own directory * Port basic features to MLv2 * Deprecate `AggregationPopover` * Fix MLv1's aggregation clause validation * Fix import * `requiresField` → `requiresColumn` * Add basic tests for `AggregationPicker` * Add `findAggregationOperator` test utility * Add basic tests for `AggregateStep` * Make stage index explicit * Highlight selections, add picker back button * Use long display name for aggregation clauses * Remove `React` imports * Enable temporal bucketing * Use aggregation JS wrappers * Fix `Icon` import * Patch unit test according to BE changes * Fix E2E test * Simplify `Aggregation's` `dimension` method * Fix formatting * Migrate notebook aggregations to MLv2 — Metrics (2) (#31528) * Add basic legacy metrics support * Don't show archived metrics * Highlight selected metric * Extract `AggregationPopover` in notebook step * Fix `onClose` prop * Fix icon * Add missing metric types * Use MLv2 to manage metrics * Remove legacy query logic * Add `Aggregatable` type * Migrate notebook aggregations to MLv2 — Inline expressions (3) (#31529) * Revert "Remove legacy query logic" This reverts commit 722804d. * Use HTML labels in `ExpressionWidget` * Support inline expressions for aggregations * Fix import * Fix picker navigation * Fix type error * Remove `ts-expect-error` * Migrate notebook aggregations to MLv2 — Disable MLv1 validation (4) (#31530)
* Migrate notebook aggregations to MLv2 — Operators (1) (#31527) * Add aggregation types * Add aggregation TypeScript wrappers * Move `AggregateStep` to its own directory * Port basic features to MLv2 * Deprecate `AggregationPopover` * Fix MLv1's aggregation clause validation * Fix import * `requiresField` → `requiresColumn` * Add basic tests for `AggregationPicker` * Add `findAggregationOperator` test utility * Add basic tests for `AggregateStep` * Make stage index explicit * Highlight selections, add picker back button * Use long display name for aggregation clauses * Remove `React` imports * Enable temporal bucketing * Use aggregation JS wrappers * Fix `Icon` import * Patch unit test according to BE changes * Fix E2E test * Simplify `Aggregation's` `dimension` method * Fix formatting * Migrate notebook aggregations to MLv2 — Metrics (2) (#31528) * Add basic legacy metrics support * Don't show archived metrics * Highlight selected metric * Extract `AggregationPopover` in notebook step * Fix `onClose` prop * Fix icon * Add missing metric types * Use MLv2 to manage metrics * Remove legacy query logic * Add `Aggregatable` type * Migrate notebook aggregations to MLv2 — Inline expressions (3) (#31529) * Revert "Remove legacy query logic" This reverts commit 722804d. * Use HTML labels in `ExpressionWidget` * Support inline expressions for aggregations * Fix import * Fix picker navigation * Fix type error * Remove `ts-expect-error` * Migrate notebook aggregations to MLv2 — Disable MLv1 validation (4) (#31530)
* Migrate notebook aggregations to MLv2 — Operators (1) (#31527) * Add aggregation types * Add aggregation TypeScript wrappers * Move `AggregateStep` to its own directory * Port basic features to MLv2 * Deprecate `AggregationPopover` * Fix MLv1's aggregation clause validation * Fix import * `requiresField` → `requiresColumn` * Add basic tests for `AggregationPicker` * Add `findAggregationOperator` test utility * Add basic tests for `AggregateStep` * Make stage index explicit * Highlight selections, add picker back button * Use long display name for aggregation clauses * Remove `React` imports * Enable temporal bucketing * Use aggregation JS wrappers * Fix `Icon` import * Patch unit test according to BE changes * Fix E2E test * Simplify `Aggregation's` `dimension` method * Fix formatting * Migrate notebook aggregations to MLv2 — Metrics (2) (#31528) * Add basic legacy metrics support * Don't show archived metrics * Highlight selected metric * Extract `AggregationPopover` in notebook step * Fix `onClose` prop * Fix icon * Add missing metric types * Use MLv2 to manage metrics * Remove legacy query logic * Add `Aggregatable` type * Migrate notebook aggregations to MLv2 — Inline expressions (3) (#31529) * Revert "Remove legacy query logic" This reverts commit 722804d. * Use HTML labels in `ExpressionWidget` * Support inline expressions for aggregations * Fix import * Fix picker navigation * Fix type error * Remove `ts-expect-error` * Migrate notebook aggregations to MLv2 — Disable MLv1 validation (4) (#31530)
* Migrate notebook aggregations to MLv2 — Operators (1) (#31527) * Add aggregation types * Add aggregation TypeScript wrappers * Move `AggregateStep` to its own directory * Port basic features to MLv2 * Deprecate `AggregationPopover` * Fix MLv1's aggregation clause validation * Fix import * `requiresField` → `requiresColumn` * Add basic tests for `AggregationPicker` * Add `findAggregationOperator` test utility * Add basic tests for `AggregateStep` * Make stage index explicit * Highlight selections, add picker back button * Use long display name for aggregation clauses * Remove `React` imports * Enable temporal bucketing * Use aggregation JS wrappers * Fix `Icon` import * Patch unit test according to BE changes * Fix E2E test * Simplify `Aggregation's` `dimension` method * Fix formatting * Migrate notebook aggregations to MLv2 — Metrics (2) (#31528) * Add basic legacy metrics support * Don't show archived metrics * Highlight selected metric * Extract `AggregationPopover` in notebook step * Fix `onClose` prop * Fix icon * Add missing metric types * Use MLv2 to manage metrics * Remove legacy query logic * Add `Aggregatable` type * Migrate notebook aggregations to MLv2 — Inline expressions (3) (#31529) * Revert "Remove legacy query logic" This reverts commit 722804d. * Use HTML labels in `ExpressionWidget` * Support inline expressions for aggregations * Fix import * Fix picker navigation * Fix type error * Remove `ts-expect-error` * Migrate notebook aggregations to MLv2 — Disable MLv1 validation (4) (#31530)
Part of #31001
The second step in the notebook editor's
AggregateStep
migration. Adds support for metrics (distinct entities that can be added in "Admin > Data Model").We're about to rework Metrics and make them model-based. It didn't make a lot of sense to port legacy metrics to MLv2, so we're still using MLv1 here. I'm open for feedback on how to approach this (look at
legacyQuery
andonChangeLegacy
props inAggregationPicker
)To Verify
/admin/datamodel/metrics
and add a few metrics/question/:id/notebook
master
Demo
CleanShot.2023-06-13.at.16.05.00.mp4