String comparison operators

[wojciechczerniak]

Description

String comparison operators =, <, >, <=, >=, <> are not supported fully. Currently, they have a naive implementation that assumes the JS will handle it correctly:

hyperformula/src/interpreter/scalar.ts

Lines 249 to 251 in 20a7d41

	export function greater(left: number | string | boolean, right: number | string | boolean, eps: number): boolean {
	if (typeof left === 'string' || typeof right === 'string') {
	return left > right

• Case sensitive/insensitive config is not used Global settings #58 (default should be insensitive)
• Culture/locale is not taken under consideration
• We do not control how special characters and accent characters are handled

A	B	=A>B (expected)	=A>B (result)
a	A	false	true
aa	AA	false	true
aA	aa	false	false
Aa	aa	false	false

Steps to reproduce

  it('should correctly compare strings', () => {
    const engine = createEngine([
      ['a', 'A', '=A1>B1'],
      ['aa', 'AA', '=A2>B2'],
      ['aA', 'aa', '=A3>B3'],
      ['Aa', 'aa', '=A4>B4'],
    ]);

    expect(engine.getCellValue('C1')).toBe(false);
    expect(engine.getCellValue('C2')).toBe(false);
    expect(engine.getCellValue('C3')).toBe(false);
    expect(engine.getCellValue('C4')).toBe(false);
  })

Links

Gnumeric implementation: https://gitlab.gnome.org/GNOME/gnumeric/blob/master/src/value.c#L1674-1685 (they are normalized)
JS comparators: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators
JS localeCompare: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/localeCompare

[izulin]

solved #163

[wojciechczerniak]

@izulin #163 handles case sensitivity 👍 But what about other mentioned use cases? Have you tested comparison with Unicode characters, how are compared strings in Chinese or Turkish language with accent characters?

[izulin]

@izulin #163 handles case sensitivity 👍 But what about other mentioned use cases? Have you tested comparison with Unicode characters, how are compared strings in Chinese or Turkish language with accent characters?

see #203 and partial solution

[aninde]

Supplement for case sensitive
Expected result based on Libra Office (spreadsheet with case sensitive)

The action in the Libra office with case sensitivity was a reference point for me to report this error.
In each test below, the commented lines return incorrect results.

hyperformula/test/conformance/text-operations.spec.ts

Lines 229 to 246 in 1af0bc3

	xit('"A", "a" should be supported by all operators with case sensitive', () => {
	const engine = HyperFormula.buildFromArray([
	['A', 'a', ...data],
	], new Config({ caseSensitive : true }))
	expect(engine.getCellValue(adr('C1'))).toEqual(false) // EQUAL
	//expect(engine.getCellValue(adr('D1'))).toEqual(true) // GT
	//expect(engine.getCellValue(adr('E1'))).toEqual(false) // LT
	//expect(engine.getCellValue(adr('F1'))).toEqual(true) // GTE
	//expect(engine.getCellValue(adr('G1'))).toEqual(false) // LTE
	expect(engine.getCellValue(adr('H1'))).toEqual(true) // NOT EQUAL
	expect(engine.getCellValue(adr('I1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) // ADD
	expect(engine.getCellValue(adr('J1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) // SUB
	expect(engine.getCellValue(adr('K1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) // MULT
	expect(engine.getCellValue(adr('L1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) //DIV
	expect(engine.getCellValue(adr('M1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) //EXP
	expect(engine.getCellValue(adr('N1'))).toEqual('Aa') // CONCAT
	})

hyperformula/test/conformance/text-operations.spec.ts

Lines 191 to 208 in 1af0bc3

	xit('"AAa", "aaa" should be supported by all operators with case sensitive', () => {
	const engine = HyperFormula.buildFromArray([
	['AAa', 'aaa', ...data],
	], new Config({ caseSensitive : true }))
	expect(engine.getCellValue(adr('C1'))).toEqual(false) // EQUAL
	//expect(engine.getCellValue(adr('D1'))).toEqual(true) // GT
	//expect(engine.getCellValue(adr('E1'))).toEqual(false) // LT
	//expect(engine.getCellValue(adr('F1'))).toEqual(true) // GTE
	//expect(engine.getCellValue(adr('G1'))).toEqual(false) // LTE
	expect(engine.getCellValue(adr('H1'))).toEqual(true) // NOT EQUAL
	expect(engine.getCellValue(adr('I1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) // ADD
	expect(engine.getCellValue(adr('J1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) // SUB
	expect(engine.getCellValue(adr('K1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) // MULT
	expect(engine.getCellValue(adr('L1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) //DIV
	expect(engine.getCellValue(adr('M1'))).toEqual(new DetailedCellError(new CellError(ErrorType.VALUE), '#VALUE!')) //EXP
	expect(engine.getCellValue(adr('N1'))).toEqual('AAaaaa') // CONCAT
	})

[izulin]

In each test below, the commented lines return incorrect results.

Is it also wrong if you test it in branch pu/issue-130?

[aninde]

I merge to my branch conformance, branch pu/issue-130 and the results are still the same like in my above #130 (comment). It is not fixed by it.

However, in the same environment another test passes. 'jaźŃ' compared with ąęćńóśżź. Reported here #225 (comment)

[izulin]

I merge to my branch conformance, branch pu/issue-130 and the results are still the same like in my above #130 (comment). It is not fixed by it.

However, in the same environment another test passes. 'jaźŃ' compared with ąęćńóśżź. Reported here #225 (comment)

should be fixed with latest commit here: #203

[aninde]

Yes, it is fixed. I checked it with after merge conformance(my testing branch) with a branch pu/issue-130.

Testing individual commits makes the process very difficult and promotes regression, so I prefer it to be an exception to the rule :) Anyway I need to verify it on develop after merge of pu/issue-130. I wouldn't close this issue until it is verified on development.

[izulin]

Yes, it is fixed. I checked it with after merge conformance(my testing branch) with a branch pu/issue-130.

Testing individual commits makes the process very difficult and promotes regression, so I prefer it to be an exception to the rule :) Anyway I need to verify it on develop after merge of pu/issue-130. I wouldn't close this issue until it is verified on development.

Of course -- I asked just to make sure that the source of error is what I thought is the source of error is. Right now #203 is pending only because it requires version update, and I am happy that the logic there conforms to the testing branch.