Preserve signed subtraction results in select expressions

[sanikolaev]

Problem

A simple query such as:

  SELECT *, 1 - 2 AS diff FROM t;

could return:

  diff = 4294967295

instead of:

  diff = -1

This is especially annoying when you need to compare something with now(), where any positive number means the value you're comparing is greater than or less than now(), but you always get a positive number.

Fix

Special-case - in ExprParser_t::AddNodeOp():

• keep using GetWidestRet() to determine the arithmetic family
• but if subtraction infers SPH_ATTR_INTEGER, promote the result type to SPH_ATTR_BIGINT

This preserves signed output for negative subtraction results while keeping the rest of the arithmetic type flow unchanged.

[tomatolog]

As the patch changed DWORD into in64 result set for negate \ substract expression
could be better to run these tests and measure the perf and memory consumption by the daemon (RSS and the heap allocation too - one that release by daemon into OS with the malloc_trim call). Could be better to check the master version and the version with this patch applied.

The cases need 1000 runs of the queries like:

1. for large result set processing (limit 1m + OPTION max_matches = 1M) and the source int32 attr

SELECT *, 1 - 2 AS diff FROM t;
SELECT *, attr_int - 2 AS diff FROM t;

2. for full-scan passes on the large table 1M docs to make sure how the filter handles that kind of attr

SELECT *, 1 - 2 AS diff FROM t where diff<0;
SELECT *, attr_int - 2 AS diff FROM t where diff<0;

3. for grouper \ sorter handles that kind of attr

SELECT *, 1 - 2 AS diff FROM t group by diff;
SELECT *, attr_int - 2 AS diff FROM t group by diff;

Could be better to explicitly set cutoff=0 to make sure the optimizer does not early exit after filled the default limit=20 matches.
The master version processes these cases with the source int32 attribute but the patched version widen the attribute types into int64 - that could affect the all other users.

[sanikolaev]

Clarification on:

This is especially annoying when you need to compare something with now(), where any positive number means the value you're comparing is greater than or less than now(), but you always get a positive number.

Example: Imagine you use Manticore to schedule something in the future. When the time comes, an event should happen. The easiest way to do it is where now() > time, which Manticore doesn't support. now() - time diff ... where diff > 0 works, but you can't use it since the diff is always positive, even for the events in the future.

mysql> drop table if exists reminder; create table reminder(t timestamp); insert into reminder values(1, 1777954651), (2, 2777854651); select * from reminder; select now() - t diff from reminder where diff > 0;
--------------
drop table if exists reminder
--------------

Query OK, 0 rows affected (0.002 sec)

--------------
create table reminder(t timestamp)
--------------

Query OK, 0 rows affected (0.002 sec)

--------------
insert into reminder values(1, 1777954651), (2, 2777854651)
--------------

Query OK, 2 rows affected (0.000 sec)

--------------
select * from reminder
--------------

+------+------------+
| id   | t          |
+------+------------+
|    1 | 1777954651 |
|    2 | 2777854651 |
+------+------------+
2 rows in set (0.000 sec)
--- 2 out of 2 results in 73us ---

--------------
select now() - t diff from reminder where diff > 0
--------------

+------------+
| diff       |
+------------+
|    1142147 |
| 3296209443 |
+------------+
2 rows in set (0.000 sec)
--- 2 out of 2 results in 41us ---

The workaround is quite ugly:

select t, if(now() - t > 0, 1, 0) cond from reminder where cond = 1
--------------

+------------+------+
| t          | cond |
+------------+------+
| 1777954651 |    1 |
+------------+------+
1 row in set (0.000 sec)
--- 1 out of 1 results in 89us ---

In the PR:

select now() - t diff from reminder where diff > 0
--------------

+---------+
| diff    |
+---------+
| 1143739 |
+---------+

[sanikolaev]

The cases need 1000 runs of the queries like:

This script was run in docker images ghcr.io/manticoresoftware/manticoresearch:test-kit-latest and ghcr.io/manticoresoftware/manticoresearch:test-kit-fix_negative-expr on dev2:

Details

manticore-load \
--drop \
--quiet \
--batch-size=1000 \
--threads=5 \
--total=1000000 \
--init="CREATE TABLE test(attr_int int)" \
--load="INSERT INTO test(id,attr_int) VALUES(<increment>,<int/1/10>)"

manticore-load --quiet \
--threads=5 \
--total=10000 \
--load="SELECT *, 1 - 2 AS diff FROM test option cutoff=0"

manticore-load --quiet \
--threads=5 \
--total=10000 \
--load="SELECT *, attr_int - 2 AS diff FROM test option cutoff=0"

manticore-load --quiet \
--threads=5 \
--total=10000 \
--load="SELECT *, 1 - 2 AS diff FROM test where diff<0 option cutoff=0"

manticore-load --quiet \
--threads=5 \
--total=10000 \
--load="SELECT *, attr_int - 10 AS diff FROM test where diff<0 option cutoff=0"

manticore-load --quiet \
--threads=5 \
--total=10000 \
--load="SELECT *, 1 - 2 AS diff FROM test group by diff option cutoff=0"

manticore-load --quiet \
--threads=5 \
--total=10000 \
--load="SELECT *, attr_int - 10 AS diff FROM test group by diff option cutoff=0"

Results:

query	PR qps	master qps
SELECT *, 1 - 2 AS diff FROM test option cutoff=0	1170	1171
SELECT *, attr_int - 2 AS diff FROM test option cutoff=0	1354	1350
SELECT *, 1 - 2 AS diff FROM test where diff<0 option cutoff=0	784	1105
SELECT *, attr_int - 10 AS diff FROM test where diff<0 option cutoff=0	626	869
SELECT *, 1 - 2 AS diff FROM test group by diff option cutoff=0	587	583
SELECT *, attr_int - 10 AS diff FROM test group by diff option cutoff=0	480	478

So there's a difference for 2 queries, but it should be taken into consideration that the results for them differ since in the master branch diff < 0 returns nothing:

mysql> SELECT *, 1 - 2 AS diff FROM test where diff<0 option cutoff=0;
mysql>

mysql> SELECT *, attr_int - 10 AS diff FROM test where diff<0 option cutoff=0;
mysql>

while in the PR branch they return results:

mysql> SELECT *, 1 - 2 AS diff FROM test where diff<0 option cutoff=0;
+--------+----------+------+
| id     | attr_int | diff |
+--------+----------+------+
| 557001 |        7 |   -1 |
| 557002 |        3 |   -1 |
| 557003 |        5 |   -1 |
| 557004 |        2 |   -1 |
| 557005 |        9 |   -1 |
| 557006 |        8 |   -1 |
| 557007 |        7 |   -1 |
| 557008 |        7 |   -1 |
| 557009 |        4 |   -1 |
| 557010 |        3 |   -1 |
| 557011 |        8 |   -1 |
| 557012 |        3 |   -1 |
| 557013 |        4 |   -1 |
| 557014 |        7 |   -1 |
| 557015 |       10 |   -1 |
| 557016 |        1 |   -1 |
| 557017 |        5 |   -1 |
| 557018 |        4 |   -1 |
| 557019 |        7 |   -1 |
| 557020 |        9 |   -1 |
+--------+----------+------+
mysql> SELECT *, attr_int - 10 AS diff FROM test where diff<0 option cutoff=0;
+--------+----------+------+
| id     | attr_int | diff |
+--------+----------+------+
| 557001 |        7 |   -3 |
| 557002 |        3 |   -7 |
| 557003 |        5 |   -5 |
| 557004 |        2 |   -8 |
| 557005 |        9 |   -1 |
| 557006 |        8 |   -2 |
| 557007 |        7 |   -3 |
| 557008 |        7 |   -3 |
| 557009 |        4 |   -6 |
| 557010 |        3 |   -7 |
| 557011 |        8 |   -2 |
| 557012 |        3 |   -7 |
| 557013 |        4 |   -6 |
| 557014 |        7 |   -3 |
| 557016 |        1 |   -9 |
| 557017 |        5 |   -5 |
| 557018 |        4 |   -6 |
| 557019 |        7 |   -3 |
| 557020 |        9 |   -1 |
| 557021 |        3 |   -7 |
+--------+----------+------+

[sanikolaev]

Results with SINT() in the master branch:

query	PR qps	master qps	master + SINT qps
SELECT *, 1 - 2 AS diff FROM test option cutoff=0	1170	1171
SELECT *, attr_int - 2 AS diff FROM test option cutoff=0	1354	1350
SELECT *, 1 - 2 AS diff FROM test where diff<0 option cutoff=0	784	1105	752
SELECT *, attr_int - 10 AS diff FROM test where diff<0 option cutoff=0	626	869	618
SELECT *, 1 - 2 AS diff FROM test group by diff option cutoff=0	587	583
SELECT *, attr_int - 10 AS diff FROM test group by diff option cutoff=0	480	478

[sanikolaev]

As discussed in Slack, it makes sense to test a query like ... where diff<>10 \ ... where diff>10

Updated table:

query	PR qps	master qps	master + SINT qps
SELECT *, 1 - 2 AS diff FROM test option cutoff=0	1170	1171
SELECT *, attr_int - 2 AS diff FROM test option cutoff=0	1354	1350
SELECT *, 1 - 2 AS diff FROM test where diff<0 option cutoff=0	784	1105	752
SELECT *, attr_int - 10 AS diff FROM test where diff<0 option cutoff=0	626	869	618
SELECT *, 1 - 2 AS diff FROM test group by diff option cutoff=0	587	583
SELECT *, attr_int - 10 AS diff FROM test group by diff option cutoff=0	480	478
SELECT *, attr_int - 10 AS diff FROM test where diff <> 20 option cutoff=0 (run simultaneously in both containers to make the results fairer)	326	323

So still no sign of a performance degradation.

[klirichek]

too complex and redundand.
Original line can be just replaced to

DECLARE_UNARY_INT ( Expr_Sint_c,		INT64FIRST,			INT64FIRST,		INT64FIRST )

(that is - simple change INTFIRST to INT64FIRST triple.

[klirichek]

suggest to add:

	SELECT *, 1-2+2000000000+1000000001 AS diff FROM test WHERE id=10;
	SELECT *, aa-4+2000000000+1000000001 AS diff FROM test ORDER BY id ASC;
	SELECT *, 1-aa+2000000000+1000000001 AS diff FROM test ORDER BY id ASC;
	SELECT *, aa-(aa+1)+2000000000+1000000001 AS diff FROM test ORDER BY id ASC;
	SELECT *, (1-2)+aa+2000000000+1000000001 AS diff FROM test ORDER BY id ASC;
	SELECT *, (1-2)-(3-4)+2000000000+1000000001 AS diff FROM test WHERE id=10;

after this block. Comparing to current master, it also reveals prominent difference in result

[sanikolaev]

It might make sense to use functions like datediff() (or create a new one if this one doesn't fit) for operations with dates.

[sanikolaev]

It might make sense to use functions like datediff() (or create a new one if this one doesn't fit) for operations with dates.

I suggest we create a new function TIMEDIFF_SEC(), to handle this task. Additionally, we need to improve the documentation and create a short article explaining the purpose of the new function, based on the confusion described in this PR.

Specification:

Implement a new date/time function:

TIMEDIFF_SEC(A)
TIMEDIFF_SEC(A, B)

Behavior

• One argument:
  Returns the difference in whole seconds between the Unix timestamp A and the current time.
  Equivalent to:
  A - NOW()
  Positive values indicate A is in the future, negative values indicate it is in the past.

• Two arguments:
  Returns the difference in whole seconds between the Unix timestamps A and B.
  Equivalent to:
  A - B
  Positive values indicate A is later than B, negative values indicate it is earlier.

Arguments

• A: Unix timestamp (seconds since the Unix epoch).
• B: Unix timestamp (seconds since the Unix epoch).

Return type

• Signed 64-bit integer representing the difference in whole seconds.

Examples

SELECT TIMEDIFF_SEC(1782993600);
-- Seconds until (or since) the specified timestamp relative to NOW()

SELECT TIMEDIFF_SEC(1782993600, 1782993570);
-- 30

SELECT TIMEDIFF_SEC(1782993570, 1782993600);
-- -30

Notes

• The sign of the result must always follow the expression A - B (or A - NOW() for the single-argument form).
• Ideally, it should be possible to use select ... where timediff_sec(attr) > 0, not just select ... timediff_sec(...) diff ... where diff > 0, but it's optional.