Skip to content

Commit bc05611

Browse files
pregnantboypregnantboy
authored
fix: numeric comparisons for tiles v2 (#1180)
### Summary Fix ed numeric comparisons for greater than, less than, and equals operators, to follow Tiles v1 logic ### What changed? - When filter by >, >=, < and <= AND the value to compare against is a number (checked using NaN), we will cast the stored values in the column to a number and compare using the operators - If vlaue to compare against is Not a Number, we will use the default lexicographical comparison in postgres. ### How to test? - Create a tile, in one of the columns, add numbers of different length e.g. 5,10,11,27,200, mixed with some strings - Use `find single row` numeric comparisons and test
1 parent 5d9efdb commit bc05611

File tree

4 files changed

+242
-23
lines changed

4 files changed

+242
-23
lines changed

‎packages/backend/src/models/tiles/pg/__tests__/helpers.test.ts‎

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
import { describe, expect, it } from 'vitest'
2+
3+
import { isValidNumericString } from '../helpers'
4+
5+
describe('pg tiles helpers', () => {
6+
it.each([
7+
['123'],
8+
['-123'],
9+
['0'],
10+
['5'],
11+
['0.123'],
12+
['-0.123'],
13+
['123.456'],
14+
['-123.456'],
15+
['-0.0000001'],
16+
['0.00'],
17+
])('should return true if the string is a valid numeric string', (value) => {
18+
expect(isValidNumericString(value)).toBe(true)
19+
})
20+
21+
it.each([
22+
['not a number'],
23+
['123.456.789'],
24+
['-.213'],
25+
['+23'],
26+
['00.379'],
27+
['01'],
28+
['00'],
29+
['00.00'],
30+
['1e6'],
31+
['99-99'],
32+
['--12'],
33+
])(
34+
'should return false if the string is not a valid numeric string',
35+
(value) => {
36+
expect(isValidNumericString(value)).toBe(false)
37+
},
38+
)
39+
})

‎packages/backend/src/models/tiles/pg/__tests__/table-row-functions.itest.ts‎

Lines changed: 161 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -386,18 +386,23 @@ describe('table-row-functions', () => {
386386
describe('getTableRows', () => {
387387
let dataArray: Record<string, string>[]
388388
let rowIds: string[]
389+
const NUM_ROWS = 9
389390
beforeEach(async () => {
390391
// Add test rows for filtering tests
391-
dataArray = new Array(5).fill(0).map(() =>
392+
dataArray = new Array(NUM_ROWS).fill(0).map(() =>
392393
generateMockTableRowData({
393394
columnIds: dummyColumnIds,
394395
}),
395396
)
396-
dataArray[0][dummyColumnIds[3]] = '0'
397+
dataArray[0][dummyColumnIds[3]] = '5'
397398
dataArray[1][dummyColumnIds[3]] = '10'
398399
dataArray[2][dummyColumnIds[3]] = '20'
399400
dataArray[3][dummyColumnIds[3]] = '30'
400401
dataArray[4][dummyColumnIds[3]] = '40'
402+
dataArray[5][dummyColumnIds[3]] = '140'
403+
dataArray[6][dummyColumnIds[3]] = 'abc'
404+
dataArray[7][dummyColumnIds[3]] = 'def'
405+
dataArray[8][dummyColumnIds[3]] = 'ghi'
401406
rowIds = await createTableRows({
402407
tableId: dummyTable.id,
403408
dataArray,
@@ -406,7 +411,7 @@ describe('table-row-functions', () => {
406411

407412
it('should return all rows when no filters are specified', async () => {
408413
const result = await getTableRows({ tableId: dummyTable.id })
409-
expect(result.rows).toHaveLength(5)
414+
expect(result.rows).toHaveLength(NUM_ROWS)
410415
})
411416

412417
it('should return only specified columns', async () => {
@@ -415,7 +420,7 @@ describe('table-row-functions', () => {
415420
columnIds: [dummyColumnIds[0]],
416421
})
417422

418-
expect(result.rows).toHaveLength(5)
423+
expect(result.rows).toHaveLength(NUM_ROWS)
419424
expect(Object.keys(result.rows[0].data)).toEqual([dummyColumnIds[0]])
420425
})
421426

@@ -433,6 +438,20 @@ describe('table-row-functions', () => {
433438

434439
expect(result.rows).toHaveLength(1)
435440
expect(result.rows[0].rowId).toBe(rowIds[0])
441+
442+
const result2 = await getTableRows({
443+
tableId: dummyTable.id,
444+
filters: [
445+
{
446+
columnId: dummyColumnIds[3],
447+
operator: TableRowFilterOperator.Equals,
448+
value: '5',
449+
},
450+
],
451+
})
452+
453+
expect(result2.rows).toHaveLength(1)
454+
expect(result2.rows[0].rowId).toBe(rowIds[0])
436455
})
437456

438457
it('should filter rows with contains operator', async () => {
@@ -458,42 +477,171 @@ describe('table-row-functions', () => {
458477
{
459478
columnId: dummyColumnIds[3],
460479
operator: TableRowFilterOperator.GreaterThan,
461-
value: '25',
480+
value: '20',
481+
},
482+
],
483+
})
484+
485+
expect(result.rows).toHaveLength(3)
486+
expect(result.rows.map((r) => r.rowId).sort()).toEqual([
487+
rowIds[3],
488+
rowIds[4],
489+
rowIds[5],
490+
])
491+
492+
const result2 = await getTableRows({
493+
tableId: dummyTable.id,
494+
filters: [
495+
{
496+
columnId: dummyColumnIds[3],
497+
operator: TableRowFilterOperator.GreaterThan,
498+
value: 'abc',
462499
},
463500
],
464501
})
465502

466-
expect(result.rows).toHaveLength(2)
503+
expect(result2.rows).toHaveLength(2)
504+
expect(result2.rows.map((r) => r.rowId).sort()).toEqual([
505+
rowIds[7],
506+
rowIds[8],
507+
])
508+
})
509+
510+
it('should filter rows with greaterThanOrEquals operator', async () => {
511+
const result = await getTableRows({
512+
tableId: dummyTable.id,
513+
filters: [
514+
{
515+
columnId: dummyColumnIds[3],
516+
operator: TableRowFilterOperator.GreaterThanOrEquals,
517+
value: '20',
518+
},
519+
],
520+
})
521+
522+
expect(result.rows).toHaveLength(4)
467523
expect(result.rows.map((r) => r.rowId).sort()).toEqual([
524+
rowIds[2],
468525
rowIds[3],
469526
rowIds[4],
527+
rowIds[5],
528+
])
529+
530+
const result2 = await getTableRows({
531+
tableId: dummyTable.id,
532+
filters: [
533+
{
534+
columnId: dummyColumnIds[3],
535+
operator: TableRowFilterOperator.GreaterThanOrEquals,
536+
value: 'abc',
537+
},
538+
],
539+
})
540+
541+
expect(result2.rows).toHaveLength(3)
542+
expect(result2.rows.map((r) => r.rowId).sort()).toEqual([
543+
rowIds[6],
544+
rowIds[7],
545+
rowIds[8],
470546
])
471547
})
472548

549+
it('should filter rows with lessThan operator', async () => {
550+
const result = await getTableRows({
551+
tableId: dummyTable.id,
552+
filters: [
553+
{
554+
columnId: dummyColumnIds[3],
555+
operator: TableRowFilterOperator.LessThan,
556+
value: '30',
557+
},
558+
],
559+
})
560+
561+
expect(result.rows).toHaveLength(3)
562+
expect(result.rows.map((r) => r.rowId).sort()).toEqual([
563+
rowIds[0],
564+
rowIds[1],
565+
rowIds[2],
566+
])
567+
568+
const result2 = await getTableRows({
569+
tableId: dummyTable.id,
570+
filters: [
571+
{
572+
columnId: dummyColumnIds[3],
573+
operator: TableRowFilterOperator.LessThan,
574+
value: 'aaa',
575+
},
576+
],
577+
})
578+
579+
expect(result2.rows).toHaveLength(6)
580+
expect(result2.rows.map((r) => r.rowId).sort()).toEqual(
581+
rowIds.slice(0, -3),
582+
)
583+
})
584+
585+
it('should filter rows with lessThanOrEquals operator', async () => {
586+
const result = await getTableRows({
587+
tableId: dummyTable.id,
588+
filters: [
589+
{
590+
columnId: dummyColumnIds[3],
591+
operator: TableRowFilterOperator.LessThanOrEquals,
592+
value: '20',
593+
},
594+
],
595+
})
596+
597+
expect(result.rows).toHaveLength(3)
598+
expect(result.rows.map((r) => r.rowId).sort()).toEqual([
599+
rowIds[0],
600+
rowIds[1],
601+
rowIds[2],
602+
])
603+
604+
const result2 = await getTableRows({
605+
tableId: dummyTable.id,
606+
filters: [
607+
{
608+
columnId: dummyColumnIds[3],
609+
operator: TableRowFilterOperator.LessThanOrEquals,
610+
value: 'abc',
611+
},
612+
],
613+
})
614+
615+
expect(result2.rows).toHaveLength(7)
616+
expect(result2.rows.map((r) => r.rowId).sort()).toEqual(
617+
rowIds.slice(0, -2),
618+
)
619+
})
620+
473621
it('should return rows with pagination using scanLimit and cursor', async () => {
474622
// First page
475623
const firstPage = await getTableRows({
476624
tableId: dummyTable.id,
477-
scanLimit: 2,
625+
scanLimit: 4,
478626
})
479627

480-
expect(firstPage.rows).toHaveLength(2)
628+
expect(firstPage.rows).toHaveLength(4)
481629
expect(firstPage.stringifiedCursor).not.toBeNull()
482630

483631
// Second page
484632
const secondPage = await getTableRows({
485633
tableId: dummyTable.id,
486-
scanLimit: 2,
634+
scanLimit: 4,
487635
stringifiedCursor: firstPage.stringifiedCursor,
488636
})
489637

490-
expect(secondPage.rows).toHaveLength(2)
638+
expect(secondPage.rows).toHaveLength(4)
491639
expect(secondPage.stringifiedCursor).not.toBeNull()
492640

493641
// Third page
494642
const thirdPage = await getTableRows({
495643
tableId: dummyTable.id,
496-
scanLimit: 2,
644+
scanLimit: 4,
497645
stringifiedCursor: secondPage.stringifiedCursor,
498646
})
499647

@@ -509,8 +657,8 @@ describe('table-row-functions', () => {
509657

510658
// Since rows are ordered by rowId, which follows creation order,
511659
// we expect the names to be in reverse creation order
512-
expect(result.rows[0].rowId).toBe(rowIds[4])
513-
expect(result.rows[result.rows.length - 1].rowId).toBe(rowIds[0])
660+
expect(result.rows[0].rowId).toBe(rowIds[NUM_ROWS - 1])
661+
expect(result.rows[NUM_ROWS - 1].rowId).toBe(rowIds[0])
514662
})
515663
})
516664

‎packages/backend/src/models/tiles/pg/helpers.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,11 @@
1+
// This regex is used to validate that a string is a valid number
2+
// It matches strings that start with an optional minus sign, followed by an optional zero,
3+
// or any number from 1 to 9 followed by any number of digits, optionally followed by a decimal point and any number of digits.
4+
// Examples: "123", "-456", "0.789", "-0.123", "123.456", "-456.789"
5+
6+
export const VALID_NUMBER_REGEX_STRING = '^-?(0|[1-9]\\d*)(\\.\\d+)?$'
7+
export const VALID_NUMBER_REGEX = new RegExp(VALID_NUMBER_REGEX_STRING)
8+
9+
export function isValidNumericString(value: string): boolean {
10+
return VALID_NUMBER_REGEX.test(value) && !isNaN(+value)
11+
}

‎packages/backend/src/models/tiles/pg/table-row-functions.ts‎

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ import {
1717
UpdateRowInput,
1818
} from '../types'
1919

20+
import { isValidNumericString, VALID_NUMBER_REGEX_STRING } from './helpers'
21+
2022
function formatTableRow(
2123
row: Record<string, string>,
2224
tableId: string,
@@ -127,7 +129,7 @@ export const patchTableRow = async ({
127129
+value,
128130
]),
129131
)
130-
.where(key, '~', '^[-+]?\\d*\\.?\\d+$')
132+
.where(key, '~', VALID_NUMBER_REGEX_STRING)
131133
},
132134
)
133135

@@ -144,7 +146,7 @@ export const patchTableRow = async ({
144146
+value,
145147
]),
146148
)
147-
.where(key, '~', '^[-+]?\\d*\\.?\\d+$')
149+
.where(key, '~', VALID_NUMBER_REGEX_STRING)
148150
},
149151
)
150152

@@ -190,6 +192,12 @@ function addFiltersToQuery(
190192
query: Knex.QueryBuilder,
191193
filters: TableRowFilter[],
192194
) {
195+
const NumericOperators = {
196+
[TableRowFilterOperator.GreaterThan]: '>',
197+
[TableRowFilterOperator.GreaterThanOrEquals]: '>=',
198+
[TableRowFilterOperator.LessThan]: '<',
199+
[TableRowFilterOperator.LessThanOrEquals]: '<=',
200+
}
193201
for (const filter of filters) {
194202
switch (filter.operator) {
195203
case TableRowFilterOperator.Equals:
@@ -199,17 +207,30 @@ function addFiltersToQuery(
199207
query.where(filter.columnId, 'ilike', `%${filter.value}%`)
200208
break
201209
case TableRowFilterOperator.GreaterThan:
202-
query.where(filter.columnId, '>', filter.value)
203-
break
204210
case TableRowFilterOperator.GreaterThanOrEquals:
205-
query.where(filter.columnId, '>=', filter.value)
206-
break
207211
case TableRowFilterOperator.LessThan:
208-
query.where(filter.columnId, '<', filter.value)
209-
break
210-
case TableRowFilterOperator.LessThanOrEquals:
211-
query.where(filter.columnId, '<=', filter.value)
212+
case TableRowFilterOperator.LessThanOrEquals: {
213+
// if the value to compare against is a numeric string number,
214+
// we cast the stored values to a number and compare numerically
215+
if (isValidNumericString(filter.value)) {
216+
query
217+
.where(filter.columnId, '~', VALID_NUMBER_REGEX_STRING)
218+
.andWhere(
219+
tilesClient.raw('??::numeric', [filter.columnId]),
220+
NumericOperators[filter.operator],
221+
+filter.value,
222+
)
223+
} else {
224+
// if the value to compare against is a string, we compare strings
225+
// regardless of whether the stored value is a number or a string
226+
query.where(
227+
filter.columnId,
228+
NumericOperators[filter.operator],
229+
filter.value,
230+
)
231+
}
212232
break
233+
}
213234
case TableRowFilterOperator.IsEmpty:
214235
query.where((builder) => {
215236
builder.whereNull(filter.columnId).orWhere(filter.columnId, '')

0 commit comments

Comments
 (0)