Do we know why the precision is 982?

Seems somehow related to this (#11151) because the values in the cast in this plan change depending on the values there. But even removing those overrides altogether (deriveDecimal*Type) results in a cast of DECIMAL(1000, 980) being added so there's something else going on here from the Calcite side as well. a.col4 is a BIG_DECIMAL type column to which we're applying the default scale and precision of 1000 (each); if we use an INTEGER column instead (a.col3) the cast used is DECIMAL(19, 1) which makes sense because on the other side we're multiplying 0.5 with a BIGINT value (result of the sum).

The scale is 982 btw, the precision is 1000 here.

I found out that this is actually coming from type coercion during the SQL -> Rel conversion phase, where the casts are being applied because the comparison (>) is being made on different types. One is DECIMAL(1000, 1000) (the BigDecimal column using default scale / precision) and the other is DECIMAL(19, 1) (0.5 is DECIMAL(2, 1) and we're multiplying it with a BIGINT value). The logic that computes the resultant precision and scale when coercing two decimal types is here - https://github.com/apache/calcite/blob/9b51667eebc825d5d3ece72cfcdb5d2efaa2d56f/core/src/main/java/org/apache/calcite/sql/type/SqlTypeFactoryImpl.java#L460-L498.

It's a little convoluted, but the resultant scale of 982 is arrived at like this:

Math.min(maxScale, Math.min(Math.max(s1, s2), maxPrecision - Math.min(maxPrecision, Math.max(p1 - s1, p2 - s2))))

So this unexpected value does seem to be because we're using the same max and default value for scale and precision, but I'm not convinced that this is an issue given the large values we're using. For reference, Calcite seems to be using 0 as the default scale for DECIMAL types and max precision for default precision (which is 19 in Calcite).

I don't think this is correct. IICU this means our default is DECIMAL(1000, 2000), which means use a 1000 digits as total number of digits (both before and after the decimal point) and 1000 digits to the right of the decimal point. Therefore we could only store values in the range of (-1, 1).

Also, I don't know the implications these digits may have, but in BigDecimal scales from 1 to 10 (or -1 to -10, but we don't care about them) are more efficient than the others

This is a good question - TBH, I just copied this part from @Jackie-Jiang's draft PR for the 1.38.0 upgrade - #15141. I can spend some time to study all the implications here and figure out the best way forward.

Okay so using a smaller default scale / precision in the range of 1-10 doesn't seem like a great idea. That would truncate values unnecessarily. For instance, if we used a default scale of 2, SELECT CAST(123.4567 AS DECIMAL) FROM mytable LIMIT 1; would return 123.45 - Postgres returns 123.4567 for reference. So I think it makes sense to use our max scale / precision as the default scale / precision as well to avoid unnecessarily truncating / rounding values?

Also using a max / default precision and scale of 1000 doesn't mean we're limited to values in the range of (-1, 1). That would only be the case if we're using 1000 digits after the decimal point. But for instance, a value like 1234567890.0123456789 that has precision 20 and scale 10 can be represented just fine as well.

On a side note, the weird and IMO inexplicable logic we have here causes strange and unexpected results - if a decimal literal's precision is <= 30 (with non-zero scale), we use Double instead of BigDecimal leading to imprecision, so we actually see perfectly accurate results only if using numbers with precision > 30. But this is orthogonal to the Calcite upgrade and we can discuss / solve this separately.

Edit: Another important point is that the default default DECIMAL scale of 0 actually does mean a 0 scale, and not unlimited - i.e., CAST(123.4567 AS DECIMAL) would become 123.

Btw our default here is actually DECIMAL(1000, 1000) - precision of 1000, and scale of 1000. DECIMAL(1000, 2000) is impossible because scale can never be larger than precision. Our default means that we can have a total of 1000 significant digits, but also up to 1000 digits after the decimal. We don't allow specifying precision and scale for our BIG_DECIMAL columns and FWIW Postgres also allows NUMERIC without explicit precision and scale to mean an "unconstrained" numeric - https://www.postgresql.org/docs/current/datatype-numeric.html#DATATYPE-NUMERIC-DECIMAL. Postgres also supports a maximum specified precision of 1000 (and scale can be between -1000 to 1000).

Sorry, I wanted to say DECIMAL(1000, 1000).

Also using a max / default precision and scale of 1000 doesn't mean we're limited to values in the range of (-1, 1).

I think that is wrong. For example in Posgres:

postgres=# select cast(10 as DECIMAL(10, 10));
ERROR:  numeric field overflow
DETAIL:  A field with precision 10, scale 10 must round to an absolute value less than 1.
postgres=# select cast(10 as DECIMAL(10, 0));
 numeric 
---------
      10
(1 row)

postgres=# select cast(10 as DECIMAL(1, 0));
ERROR:  numeric field overflow
DETAIL:  A field with precision 1, scale 0 must round to an absolute value less than 10^1.
postgres=# select cast(10 as DECIMAL(2, 0));
 numeric 
---------
      10
(1 row)

postgres=# select cast(10 as DECIMAL(2, 1));
ERROR:  numeric field overflow
DETAIL:  A field with precision 2, scale 1 must round to an absolute value less than 10^1.

AFAIU DECIMAL(1000, 1000) can only represent from -0.999... to 0.999... with 1000 digits at the right of the decimal point. If that is the case we must change the default used for scale. And even if it isn't... shoudln't we use a smaller scale by default? That would be more efficient once implemented as BigDecimal

You're right, the reason it sort of works is because of further Pinot specific oddities. For small precision / scales, we use DOUBLE instead of BIG_DECIMAL for DECIMAL -

So here, the precision / scale won't be effectively enforced because we're using floating point representations. And then for larger precisions / scales as well, we don't actually enforce that while casting -

This just converts whatever type it is to the appropriate BigDecimal value without enforcing the specified precision / scales. It's basically yet another place where our support for the SQL DECIMAL type is lacking.

However, using a smaller default scale for DECIMAL will break backward compatibility for us. Take this example from one of our tests - SELECT CAST('12345678901234567890123456789.1234567890123456789' AS DECIMAL) FROM a. This currently returns 12345678901234567890123456789.1234567890123456789, but if we reduce the default scale to 10, this will become 12345678901234567890123456789.1234567890 because Calcite seems to be evaluating such calls by itself in SqlToRelConverter and Pinot's cast function won't be called at all. Are we okay with such a change?

cc - @Jackie-Jiang for an additional opinion as well.

Then, let's use other values larger than 10 without using the same value for precision and scale. I suggest using a precision that is at least twice the scale so we have enough room after and before the dot.

Then, after this PR is merged, we can test the performance implications of using smaller precision and scale. If it is significantly better to use smaller values, open a new PR where the default precision and scale can be changed at startup time.

Yeah, that makes sense. I also discussed with @Jackie-Jiang offline and in his opinion, the original intention of 1000, 1000 was actually probably to actually have 2000 precision and 1000 scale. I've updated this but it does mean that Calcite is now forcing some literals (direct cast to DECIMAL without explicit precision / scale or comparison between BIG_DECIMAL column and literals) to have 1000 scale which like you've pointed out might not be very efficient for Java's BigDecimal that is used internally by both Calcite and Pinot to represent SQL DECIMAL types.