aglinxinyuan commented on code in PR #6298: URL: https://github.com/apache/texera/pull/6298#discussion_r3555653348
########## frontend/src/app/workspace/component/dataset-selection-modal/dataset-search.util.ts: ########## @@ -0,0 +1,53 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { NzSelectItemInterface } from "ng-zorro-antd/select"; +import { DashboardDataset } from "../../../dashboard/type/dashboard-dataset.interface"; + +/** + * Whether a dataset matches a type-to-search query in the dataset picker. + * + * Matches against both the dataset name and its numeric id (shown as `#<id>` in the + * dropdown), case-insensitively, so typing either `iris` or `17` finds `#17 iris` + * (per the design decision on the proposal). An empty/whitespace query matches + * everything. + */ +export function datasetMatchesQuery( + name: string | null | undefined, + did: number | null | undefined, + query: string +): boolean { + const q = query.trim().toLowerCase(); + if (q === "") { + return true; + } + const nameMatches = (name ?? "").toLowerCase().includes(q); + const idMatches = did !== null && did !== undefined && String(did).includes(q); + return nameMatches || idMatches; Review Comment: Fixed — the filter now matches the displayed token (`#${did}`), so both `17` and `#17` work (d8ab7fb, pinned by tests in 1758373). ########## frontend/src/app/workspace/component/dataset-selection-modal/dataset-search.util.spec.ts: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { NzSelectItemInterface } from "ng-zorro-antd/select"; +import { datasetMatchesQuery, filterDatasetOption } from "./dataset-search.util"; + +describe("datasetMatchesQuery", () => { + it("matches by name, case-insensitively", () => { + expect(datasetMatchesQuery("iris", 17, "iris")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "IR")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "ris")).toBe(true); // substring + }); + + it("matches by numeric id", () => { + expect(datasetMatchesQuery("iris", 17, "17")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "1")).toBe(true); // substring of id + }); Review Comment: Added in 1758373 — `#17`, the `#1` prefix, a non-matching `#99`, and the id-only (`name = null`) case. ########## frontend/src/app/workspace/component/dataset-selection-modal/dataset-search.util.spec.ts: ########## @@ -0,0 +1,94 @@ +/** + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +import { NzSelectItemInterface } from "ng-zorro-antd/select"; +import { datasetMatchesQuery, filterDatasetOption } from "./dataset-search.util"; + +describe("datasetMatchesQuery", () => { + it("matches by name, case-insensitively", () => { + expect(datasetMatchesQuery("iris", 17, "iris")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "IR")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "ris")).toBe(true); // substring + }); + + it("matches by numeric id", () => { + expect(datasetMatchesQuery("iris", 17, "17")).toBe(true); + expect(datasetMatchesQuery("iris", 17, "1")).toBe(true); // substring of id + }); + + it("does not match when neither name nor id contains the query", () => { + expect(datasetMatchesQuery("iris", 17, "test")).toBe(false); + expect(datasetMatchesQuery("iris", 17, "99")).toBe(false); + }); + + it("treats an empty or whitespace query as matching everything", () => { + expect(datasetMatchesQuery("iris", 17, "")).toBe(true); + expect(datasetMatchesQuery("iris", 17, " ")).toBe(true); + }); + + it("trims and lowercases the query", () => { + expect(datasetMatchesQuery("Iris", 17, " IRIS ")).toBe(true); + }); + + it("handles missing name or id safely", () => { + expect(datasetMatchesQuery(null, 17, "17")).toBe(true); + expect(datasetMatchesQuery(undefined, null, "iris")).toBe(false); + expect(datasetMatchesQuery("iris", null, "iris")).toBe(true); + expect(datasetMatchesQuery("iris", undefined, "3")).toBe(false); + }); + + it("matches when the query appears in the name but not the id (and vice versa)", () => { + expect(datasetMatchesQuery("customers", 42, "cust")).toBe(true); // name only + expect(datasetMatchesQuery("customers", 42, "42")).toBe(true); // id only + }); + + it("does not partially match across the name/id boundary", () => { + // "s4" is not a substring of the name "iris" nor of the id "3" + expect(datasetMatchesQuery("iris", 3, "s4")).toBe(false); + }); +}); + +describe("filterDatasetOption", () => { + // Build an nz-select option carrying a DashboardDataset-shaped value, like the real + // dropdown does. Only the fields the filter reads (name, did) matter here. + const optionFor = (name: string, did: number): NzSelectItemInterface => + ({ nzLabel: name, nzValue: { dataset: { name, did } } }) as unknown as NzSelectItemInterface; + + it("matches by dataset name pulled from the option value", () => { + expect(filterDatasetOption("iris", optionFor("iris", 17))).toBe(true); + expect(filterDatasetOption("IR", optionFor("iris", 17))).toBe(true); + expect(filterDatasetOption("test", optionFor("iris", 17))).toBe(false); + }); + + it("matches by dataset #id pulled from the option value", () => { + expect(filterDatasetOption("17", optionFor("iris", 17))).toBe(true); + expect(filterDatasetOption("99", optionFor("iris", 17))).toBe(false); + }); Review Comment: Added in 1758373 — the adapter is now covered for `#17` (match) and `#99` (no match) typed from the UI label. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
