From 98595eefedffa955705a6256add778e8a62ae8fb Mon Sep 17 00:00:00 2001
From: Jeremy Maitin-Shepard <jbms@google.com>
Date: Wed, 27 Nov 2024 21:47:14 -0800
Subject: [PATCH] fix(segment properties): fix negative constraints on numeric
 properties (#673)

Fixes https://github.com/google/neuroglancer/issues/631.
---
 .../property_map.spec.ts                      | 356 +++++++++++++++++-
 .../property_map.ts                           |   2 +-
 2 files changed, 354 insertions(+), 4 deletions(-)

diff --git a/src/segmentation_display_state/property_map.spec.ts b/src/segmentation_display_state/property_map.spec.ts
index fbce7ba0c..51305ac3d 100644
--- a/src/segmentation_display_state/property_map.spec.ts
+++ b/src/segmentation_display_state/property_map.spec.ts
@@ -14,16 +14,18 @@
  * limitations under the License.
  */
 
-import { describe, it, expect } from "vitest";
+import { describe, test, expect } from "vitest";
 import {
   mergeSegmentPropertyMaps,
+  parseSegmentQuery,
   PreprocessedSegmentPropertyMap,
   SegmentPropertyMap,
 } from "#src/segmentation_display_state/property_map.js";
+import { DataType } from "#src/util/data_type.js";
 import { Uint64 } from "#src/util/uint64.js";
 
 describe("PreprocessedSegmentPropertyMap", () => {
-  it("handles lookups correctly", () => {
+  test("handles lookups correctly", () => {
     const map = new PreprocessedSegmentPropertyMap({
       inlineProperties: {
         ids: Uint32Array.of(5, 0, 15, 0, 20, 5),
@@ -39,7 +41,7 @@ describe("PreprocessedSegmentPropertyMap", () => {
 });
 
 describe("mergeSegmentPropertyMaps", () => {
-  it("works correctly for 2 maps", () => {
+  test("works correctly for 2 maps", () => {
     const a = new SegmentPropertyMap({
       inlineProperties: {
         ids: Uint32Array.of(5, 0, 6, 0, 8, 0),
@@ -65,3 +67,351 @@ describe("mergeSegmentPropertyMaps", () => {
     });
   });
 });
+
+describe("parseSegmentQuery", () => {
+  const map = new PreprocessedSegmentPropertyMap({
+    inlineProperties: {
+      ids: Uint32Array.of(),
+      properties: [
+        { type: "label", id: "label", values: [] },
+        {
+          type: "number",
+          dataType: DataType.INT32,
+          description: undefined,
+          id: "prop1",
+          values: Int32Array.of(),
+          bounds: [-10, 100],
+        },
+        {
+          id: "tags",
+          type: "tags",
+          tags: ["abc", "def"],
+          tagDescriptions: ["foo", "bar"],
+          values: [],
+        },
+      ],
+    },
+  });
+
+  test("handles empty query", () => {
+    expect(parseSegmentQuery(undefined, "")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "id",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles single number", () => {
+    expect(parseSegmentQuery(undefined, "123")).toMatchInlineSnapshot(`
+      {
+        "ids": [
+          "123",
+        ],
+      }
+    `);
+  });
+
+  test("handles multiple numbers", () => {
+    expect(parseSegmentQuery(undefined, "123 456")).toMatchInlineSnapshot(`
+      {
+        "ids": [
+          "123",
+          "456",
+        ],
+      }
+    `);
+  });
+
+  test("handles regular expression", () => {
+    expect(parseSegmentQuery(map, "/xyz")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [],
+        "prefix": undefined,
+        "regexp": /xyz/,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles prefix", () => {
+    expect(parseSegmentQuery(map, "xyz")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [],
+        "prefix": "xyz",
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles prefix", () => {
+    expect(parseSegmentQuery(map, "xyz")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [],
+        "prefix": "xyz",
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles numeric > comparison", () => {
+    expect(parseSegmentQuery(map, "prop1>5")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [
+          {
+            "bounds": [
+              6,
+              100,
+            ],
+            "fieldId": "prop1",
+          },
+        ],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles numeric >= comparison", () => {
+    expect(parseSegmentQuery(map, "prop1>=5")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [
+          {
+            "bounds": [
+              5,
+              100,
+            ],
+            "fieldId": "prop1",
+          },
+        ],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles numeric = comparison", () => {
+    expect(parseSegmentQuery(map, "prop1=5")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [
+          {
+            "bounds": [
+              5,
+              5,
+            ],
+            "fieldId": "prop1",
+          },
+        ],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles numeric >= comparison", () => {
+    expect(parseSegmentQuery(map, "prop1>=5")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [
+          {
+            "bounds": [
+              5,
+              100,
+            ],
+            "fieldId": "prop1",
+          },
+        ],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles numeric > comparison", () => {
+    expect(parseSegmentQuery(map, "prop1>5")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [
+          {
+            "bounds": [
+              6,
+              100,
+            ],
+            "fieldId": "prop1",
+          },
+        ],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles numeric > comparison negative", () => {
+    expect(parseSegmentQuery(map, "prop1>-5")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [
+          {
+            "bounds": [
+              -4,
+              100,
+            ],
+            "fieldId": "prop1",
+          },
+        ],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles sort field", () => {
+    expect(parseSegmentQuery(map, ">prop1")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [],
+        "includeTags": [],
+        "numericalConstraints": [],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "prop1",
+            "order": ">",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles column inclusions", () => {
+    expect(parseSegmentQuery(map, "|prop1")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [],
+        "includeColumns": [
+          "prop1",
+        ],
+        "includeTags": [],
+        "numericalConstraints": [],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+
+  test("handles tags", () => {
+    expect(parseSegmentQuery(map, "#abc -#def")).toMatchInlineSnapshot(`
+      {
+        "excludeTags": [
+          "def",
+        ],
+        "includeColumns": [],
+        "includeTags": [
+          "abc",
+        ],
+        "numericalConstraints": [],
+        "prefix": undefined,
+        "regexp": undefined,
+        "sortBy": [
+          {
+            "fieldId": "label",
+            "order": "<",
+          },
+        ],
+      }
+    `);
+  });
+});
diff --git a/src/segmentation_display_state/property_map.ts b/src/segmentation_display_state/property_map.ts
index aa1c79853..943bee711 100644
--- a/src/segmentation_display_state/property_map.ts
+++ b/src/segmentation_display_state/property_map.ts
@@ -697,7 +697,7 @@ export function parseSegmentQuery(
       continue;
     }
     const constraintMatch = word.match(
-      /^([a-zA-Z][a-zA-Z0-9_]*)(<|<=|=|>=|>)([0-9.].*)$/,
+      /^([a-zA-Z][a-zA-Z0-9_]*)(<|<=|=|>=|>)(-?[0-9.].*)$/,
     );
     if (constraintMatch !== null) {
       let fieldId = constraintMatch[1].toLowerCase();