Polymorphic binary arithmetic scalar functions

[yashmayya]

• Implement polymorphic binary arithmetic scalar functions (+, -, *, /) based on the framework added in Refactor function registry for multi-stage engine #13573 (similar to Support polymorphic scalar comparison functions in the multi-stage query engine #13711).
• Before this, all such functions relied on double based calculations regardless of the input operand types. After this, functions involving only integer / long values will use a long based calculation.
• This solves a whole class of bugs along with bringing standard SQL-like semantics.
• The added test shows a simple case that would've failed without these changes. A query like: SELECT * FROM mytable WHERE ts BETWEEN now() - 1000 AND now() fails on the v1 single-stage query engine without these changes. The reason is that during query compilation, the CompileTimeFunctionsInvoker query rewriter will replace now() - 1000 with the equivalent double value since that's the only available implementation of the minus function -

  pinot/pinot-common/src/main/java/org/apache/pinot/common/function/scalar/ArithmeticFunctions.java

  Lines 38 to 41 in 852e4f3

  	@ScalarFunction(names = {"sub", "minus"})
  	public static double minus(double a, double b) {
  	return a - b;
  	}

• This fails later on either in the MergeRangeFilterOptimizer in the broker or in the ColumnValueSegmentPruner in the server. The root cause of both failures is that the literal representing the range bound gets converted to a string (see here and here for instance) and later when converting back to a LONG, we run into an issue since the string could be the scientific representation of the double value:

Exception: java.lang.IllegalArgumentException: Cannot convert value: '1.726807808874E12' to type: LONG
        at org.apache.pinot.spi.data.FieldSpec$DataType.convertInternal(FieldSpec.java:724)
        at org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer.getComparable(MergeRangeFilterOptimizer.java:149)
        at org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer.getRange(MergeRangeFilterOptimizer.java:135)
        at org.apache.pinot.core.query.optimizer.filter.MergeRangeFilterOptimizer.optimize(MergeRangeFilterOptimizer.java:85)

org.apache.pinot.spi.exception.BadQueryRequestException: java.lang.IllegalArgumentException: Cannot convert value: '1.727344079699E12' to type: LONG
	at org.apache.pinot.core.query.pruner.ValueBasedSegmentPruner.convertValue(ValueBasedSegmentPruner.java:157)
	at org.apache.pinot.core.query.pruner.ColumnValueSegmentPruner.pruneRangePredicate(ColumnValueSegmentPruner.java:176)
	at org.apache.pinot.core.query.pruner.ColumnValueSegmentPruner.pruneSegmentWithPredicate(ColumnValueSegmentPruner.java:88)
	at org.apache.pinot.core.query.pruner.ValueBasedSegmentPruner.pruneSegment(ValueBasedSegmentPruner.java:144)

• We should also look into fixing the above root cause by using a type specific bounds in the range predicates instead of a string value.

[yashmayya]

I've removed the polymorphic division function implementation because that breaks backward compatibility for the v1 query engine. select 1 / 2 from mytable currently returns 0.5 in v1 and with the polymorphic function changes it would return 0 (which is actually the SQL compliant result, and is also the returned result in the v2 engine with or without these changes due to Calcite based return type inference of the standard division operator).

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 75.75758% with 16 lines in your changes missing coverage. Please review.

Project coverage is 64.11%. Comparing base (59551e4) to head (fbd44ce).
Report is 1109 commits behind head on master.

Files with missing lines	Patch %	Lines
...unction/scalar/arithmetic/MinusScalarFunction.java	73.33%	3 Missing and 1 partial ⚠️
...function/scalar/arithmetic/MultScalarFunction.java	73.33%	3 Missing and 1 partial ⚠️
...function/scalar/arithmetic/PlusScalarFunction.java	73.33%	3 Missing and 1 partial ⚠️
...tic/PolymorphicBinaryArithmeticScalarFunction.java	60.00%	2 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##             master   #14089      +/-   ##
============================================
+ Coverage     61.75%   64.11%   +2.36%     
- Complexity      207     1538    +1331     
============================================
  Files          2436     2600     +164     
  Lines        133233   143505   +10272     
  Branches      20636    21982    +1346     
============================================
+ Hits          82274    92014    +9740     
+ Misses        44911    44722     -189     
- Partials       6048     6769     +721     

Flag	Coverage Δ
custom-integration1	100.00% <ø> (+99.99%)	⬆️
integration	100.00% <ø> (+99.99%)	⬆️
integration1	100.00% <ø> (+99.99%)	⬆️
integration2	0.00% <ø> (ø)
java-11	64.09% <75.75%> (+2.38%)	⬆️
java-21	63.98% <75.75%> (+2.36%)	⬆️
skip-bytebuffers-false	64.10% <75.75%> (+2.36%)	⬆️
skip-bytebuffers-true	63.96% <75.75%> (+36.23%)	⬆️
temurin	64.11% <75.75%> (+2.36%)	⬆️
unittests	64.11% <75.75%> (+2.36%)	⬆️
unittests1	55.82% <75.75%> (+8.93%)	⬆️
unittests2	34.53% <68.18%> (+6.80%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Jackie-Jiang]

Per the PG behavior: https://www.postgresql.org/docs/current/functions-math.html
Should it return INT for operations between INT columns?
Good catch on the division function behavior difference. In order to move towards standard SQL behavior without breaking backward compatibility, we can consider adding a query option to follow standard SQL. This way we can keep v1 behavior and also make v2 standard SQL compatible. V1 user can also migrate to standard SQL behavior.

[yashmayya]

Should it return INT for operations between INT columns?

Ideally yes, but that would break backward compatibility too (just like the division case). In PG, 2000000000 + 2000000000 returns ERROR: integer out of range whereas in Pinot it returns 4000000000.

Good catch on the division function behavior difference. In order to move towards standard SQL behavior without breaking backward compatibility, we can consider adding a query option to follow standard SQL. This way we can keep v1 behavior and also make v2 standard SQL compatible. V1 user can also migrate to standard SQL behavior.

Do you mean a specific query option for arithmetic related operations or just a generic option under which we'd bucket all such compatibility breaking but SQL compliant changes in the future? Also, the v2 behavior should already be SQL compliant because Calcite will make sure that the return type for the division operator is appropriate (and Pinot makes sure to cast internal values based on the expected return type of an operator).

[yashmayya]

Also, if we do end up introducing such a query option in the future, we should probably make sure that the arithmetic transform functions are updated to respect the query option too. For instance, there is an AdditionTransformFunction which also always returns DOUBLE regardless of the input operand types. And this is the function that will get used instead of the scalar function (that is being updated here) if there's a query like SELECT intCol + 5 FROM mytable.

[Jackie-Jiang]

I was thinking adding a general query option to turn v1 engine into standard sql compatible mode, so that v2 engine can enable it and get standard sql behavior. Even if we cast after getting the result, it might already lose precision and give inaccurate result.

Currently, when adding 2 ints, will v2 engine return int? For division, PG always round towards 0, which is not followed if we cast the result after execution.

[Jackie-Jiang]

Basically we want a general way so that we can add standard SQL support without breaking existing v1 queries

[yashmayya]

Even if we cast after getting the result, it might already lose precision and give inaccurate result.

Yes, this is a valid point. Also, an expression like 5 / 3 + 1.5 will return 3.166666666666667 in the v2 engine (since the cast is not done after each function call), as opposed to 2.5 in Postgres.

Currently, when adding 2 ints, will v2 engine return int

Yes, it does, which seems somewhat problematic because something like 2000000000 + 2000000000 results in an integer overflow occurring and a negative integer value being returned.

I was thinking adding a general query option to turn v1 engine into standard sql compatible mode, so that v2 engine can enable it and get standard sql behavior

Basically we want a general way so that we can add standard SQL support without breaking existing v1 queries

I wasn't able to find any common standard SQL definition for such arithmetic operations. For instance, in MySQL 1 / 2 returns 0.5 and 2000000000 + 2000000000 returns 4000000000 (similar to the Pinot v1 engine, unlike Postgres or Pinot v2 engine). Other databases seem to be doing this their own way as well. If there's a consensus that we always want to do things the Postgres way going forward, then the query option should be something like usePostgresSemantics? And coming to this PR itself, I presume we'd want the improvements here with or without such a query option right? And only put some of the compatibility breaking changes (like enforcing int for arithmetic operations on int and the type specific division) behind the new query option?

[Jackie-Jiang]

We can discuss query options in a different thread. That is orthogonal to this PR

[Jackie-Jiang]

(minor) Remove @Nullable. Same for other places

[Jackie-Jiang]

Actually maybe we should register them with the standard Calcite operator SqlStdOperatorTable.MINUS?

[yashmayya]

Ah yeah, I had pushed this change to fix some test failures with direct usages of ADD, and IIRC operator alias based registering wasn't working for me locally earlier but I checked again and things look good (might've messed something up earlier). I've pushed a commit to use aliases for the standard operators and removed these overrides.