From bd44bd453f5bffc299133292a0fd62e81f900f64 Mon Sep 17 00:00:00 2001 From: Marco polo Date: Mon, 3 Feb 2025 07:31:59 -0500 Subject: [PATCH] [Selection input] Filter out kind tags from tags + Remove contains match filter as first option in empty case (#27385) ## Summary & Motivation Kind tags already show up under the `kind:` filter, so removing them from showing up under the `tag:` filter. ## How I Tested These Changes locally, didn't add a jest test for the getAttributes function because I'm in the middle of refactoring all of this logic to be more flexible/robust. The SelectionAutoComplete jest test covers the second part of this PR ![image](https://github.com/user-attachments/assets/aafe7b7d-5744-498a-ba84-698717ceafa8) --- .../ui-core/src/asset-selection/input/util.ts | 4 + .../selection/SelectionAutoCompleteVisitor.ts | 3 +- .../ui-core/src/selection/SelectionInput.tsx | 2 +- .../SelectionInputAutoCompleteResults.tsx | 32 +++-- .../__tests__/SelectionAutoComplete.test.ts | 135 ------------------ 5 files changed, 27 insertions(+), 149 deletions(-) diff --git a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/util.ts b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/util.ts index 029519a705d0b..537b59ede7c67 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/util.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/asset-selection/input/util.ts @@ -1,5 +1,6 @@ import {assertUnreachable} from '../../app/Util'; import {AssetGraphQueryItem} from '../../asset-graph/useAssetGraphData'; +import {isKindTag} from '../../graph/KindTags'; import {buildRepoPathForHuman} from '../../workspace/buildRepoAddress'; export const getAttributesMap = (assets: AssetGraphQueryItem[]) => { @@ -13,6 +14,9 @@ export const getAttributesMap = (assets: AssetGraphQueryItem[]) => { assets.forEach((asset) => { assetNamesSet.add(asset.name); asset.node.tags.forEach((tag) => { + if (isKindTag(tag)) { + return; + } if (tag.key && tag.value) { // We add quotes around the equal sign here because the auto-complete suggestion already wraps the entire value in quotes. // So wer end up with tag:"key"="value" as the final suggestion diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteVisitor.ts b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteVisitor.ts index cdfb4b887070f..1c337ad6f2053 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteVisitor.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionAutoCompleteVisitor.ts @@ -30,7 +30,7 @@ import { const DEFAULT_TEXT_CALLBACK = (value: string) => value; // set to true for debug output if desired -const DEBUG = true; +const DEBUG = false; export type Suggestion = | { @@ -99,7 +99,6 @@ export class SelectionAutoCompleteVisitor extends BaseSelectionVisitor { this.functions = functions; this.nameBase = nameBase; this.attributesWithNameBase = [ - `${this.nameBase}_substring`, this.nameBase, ...Object.keys(attributesMap).filter((name) => name !== this.nameBase), ]; diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInput.tsx b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInput.tsx index 73d6d9462153f..6001f02ee2f28 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInput.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInput.tsx @@ -330,7 +330,7 @@ export const SelectionAutoCompleteInput = ({ useResizeObserver(inputRef, adjustHeight); return ( -
+
diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInputAutoCompleteResults.tsx b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInputAutoCompleteResults.tsx index bc959bb702787..62750894baada 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInputAutoCompleteResults.tsx +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/SelectionInputAutoCompleteResults.tsx @@ -101,7 +101,7 @@ export const SelectionInputAutoCompleteResults = React.memo( text={} active={index === selectedIndex} onClick={() => onSelect(result)} - onMouseEnter={() => setSelectedIndex({current: index})} + onMouseMove={() => setSelectedIndex({current: index})} />
@@ -180,6 +180,7 @@ const SuggestionItem = ({suggestion}: {suggestion: Suggestion}) => { } else if (suggestion.type === 'parenthesis') { icon = 'curly_braces'; label = 'Parenthesis'; + value = suggestion.text; } else if (suggestion.type === 'function') { if (suggestion.displayText === 'roots()') { label = 'Roots'; @@ -194,16 +195,25 @@ const SuggestionItem = ({suggestion}: {suggestion: Suggestion}) => { icon = attributeToIcon[suggestion.displayText as Attribute]; } else if (suggestion.type === 'attribute-with-value') { - const firstColon = suggestion.displayText.indexOf(':'); - const attributeKey = suggestion.displayText.slice(0, firstColon); - const attributeValue = suggestion.displayText.slice(firstColon + 1); - label = ( - - {attributeKey}: - {attributeValue} - - ); - value = null; + // hacky approach to extract the substring filter for now..., upcoming refactor will have more robust logic. + const substringMatch = /^([a-zA-Z]+)_substring:(.+)$/.exec(suggestion.text); + if (substringMatch) { + const nameBase = substringMatch[1]!; + label = `${nameBase[0]!.toUpperCase() + nameBase.slice(1)} contains ${substringMatch[2]}`; + icon = 'magnify_glass_checked'; + value = suggestion.displayText; + } else { + const firstColon = suggestion.displayText.indexOf(':'); + const attributeKey = suggestion.displayText.slice(0, firstColon); + const attributeValue = suggestion.displayText.slice(firstColon + 1); + label = ( + + {attributeKey}: + {attributeValue} + + ); + value = null; + } } else if (suggestion.type === 'attribute-value') { label = suggestion.displayText; value = null; diff --git a/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts b/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts index d7285d3bf34f4..9c11727cf9d3d 100644 --- a/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts +++ b/js_modules/dagster-ui/packages/ui-core/src/selection/__tests__/SelectionAutoComplete.test.ts @@ -143,13 +143,6 @@ describe('createAssetSelectionHint', () => { // empty case expect(testAutocomplete('|')).toEqual({ list: [ - { - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: 'key:', @@ -263,13 +256,6 @@ describe('createAssetSelectionHint', () => { it('should handle traversal operators correctly', () => { expect(testAutocomplete('+|')).toEqual({ list: [ - { - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: 'key:', @@ -332,13 +318,6 @@ describe('createAssetSelectionHint', () => { expect(testAutocomplete('+ |')).toEqual({ from: 2, list: [ - { - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: 'key:', @@ -392,13 +371,6 @@ describe('createAssetSelectionHint', () => { expect(testAutocomplete('+|')).toEqual({ from: 1, list: [ - { - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: 'key:', @@ -473,13 +445,6 @@ describe('createAssetSelectionHint', () => { it('should handle incomplete "not" expressions', () => { expect(testAutocomplete('not|')).toEqual({ list: [ - { - text: ' key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: ' key:', displayText: 'key:', @@ -533,13 +498,6 @@ describe('createAssetSelectionHint', () => { expect(testAutocomplete('not |')).toEqual({ list: [ - { - text: 'key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: 'key:', displayText: 'key:', @@ -595,13 +553,6 @@ describe('createAssetSelectionHint', () => { it('should handle incomplete and expressions', () => { expect(testAutocomplete('key:"asset1" and |')).toEqual({ list: [ - { - text: 'key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: 'key:', displayText: 'key:', @@ -658,13 +609,6 @@ describe('createAssetSelectionHint', () => { it('should handle incomplete or expressions', () => { expect(testAutocomplete('key:"asset1" or |')).toEqual({ list: [ - { - text: 'key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: 'key:', displayText: 'key:', @@ -750,13 +694,6 @@ describe('createAssetSelectionHint', () => { testAutocomplete('sinks(key_substring:"FIVETRAN/google_ads/ad_group_history" or |)'), ).toEqual({ list: [ - { - text: 'key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: 'key:', displayText: 'key:', @@ -816,13 +753,6 @@ describe('createAssetSelectionHint', () => { ).toEqual({ list: [ // Inserts a space before the string - { - text: ' key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: ' key:', displayText: 'key:', @@ -945,14 +875,6 @@ describe('createAssetSelectionHint', () => { testAutocomplete('key:"test" or key_substring:"FIVETRAN/google_ads/ad_group_history"+ or |'), ).toEqual({ list: [ - // Inserts a space before the string - { - text: 'key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: 'key:', displayText: 'key:', @@ -1009,14 +931,6 @@ describe('createAssetSelectionHint', () => { it('suggestions inside parenthesized expression', () => { expect(testAutocomplete('(|)')).toEqual({ list: [ - // Inserts a space before the string - { - text: 'key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: 'key:', displayText: 'key:', @@ -1073,13 +987,6 @@ describe('createAssetSelectionHint', () => { it('suggestions outside parenthesized expression before', () => { expect(testAutocomplete('|()')).toEqual({ list: [ - { - text: 'key_substring:', - displayText: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { text: 'key:', displayText: 'key:', @@ -1225,13 +1132,6 @@ describe('createAssetSelectionHint', () => { it('makes suggestions for IncompleteExpression inside of the ParenthesizedExpression', () => { expect(testAutocomplete('(key:tag and |)')).toEqual({ list: [ - { - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: 'key:', @@ -1286,13 +1186,6 @@ describe('createAssetSelectionHint', () => { expect(testAutocomplete('(key:tag and|)')).toEqual({ list: [ - { - displayText: 'key_substring:', - text: ' key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: ' key:', @@ -1347,13 +1240,6 @@ describe('createAssetSelectionHint', () => { expect(testAutocomplete('(key:tag and| )')).toEqual({ list: [ - { - displayText: 'key_substring:', - text: ' key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: ' key:', @@ -1428,13 +1314,6 @@ describe('createAssetSelectionHint', () => { ).toEqual({ from: 35, list: [ - { - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: 'key:', @@ -1563,13 +1442,6 @@ describe('createAssetSelectionHint', () => { ).toEqual({ from: 54, list: [ - { - attributeName: 'key_substring', - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - nameBase: true, - }, { displayText: 'key:', text: 'key:', @@ -1598,13 +1470,6 @@ describe('createAssetSelectionHint', () => { expect(testAutocomplete('key:"value"+ or tag:"value"+ and owner:"owner" and |')).toEqual({ from: 51, list: [ - { - displayText: 'key_substring:', - text: 'key_substring:', - type: 'attribute', - attributeName: 'key_substring', - nameBase: true, - }, { displayText: 'key:', text: 'key:',